Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows enabling hooks for code analysis #1214

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jyeshe
Copy link
Contributor

@jyeshe jyeshe commented Oct 18, 2023

Notes for the reviewer

This prevents state.json or similar content to be pushed even temporarily to the public git repo.

Allows enabling git commit and push hooks for code formatting and analysis.

@taylordowns2000 this is for the security suggest
cc @stuartc

To enable the hooks for security and code check, please run mix git_hooks.install.

Related issue

Keep going and help automating a bit GDPR (and equivalent) compliance.
Just to anticipate some checks executed by CI.

Review checklist

  • I have performed a self-review of my code
  • I have verified that all appropriate authorization policies have been implemented and tested
  • If needed, I have updated the changelog
  • Product has QA'd this feature

@jyeshe jyeshe changed the title Prevent git push without credentials Prevent git push with credentials Oct 18, 2023
Copy link
Member

@taylordowns2000 taylordowns2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jyeshe , this is super cool, but i'd suggest it doesn't belong in Lightning. Would it make more sense in OpenFn/kit or OpenFn/adaptors? (My rational is this... if a developer is writing job state and expressions on their local filesystem, it's because they're using the CLI, not Lightning. What do you think? In other words, I love the concept but think it's in the wrong repo!)

@taylordowns2000
Copy link
Member

Oooh actually, @jyeshe , this should probably go in the https://github.com/OpenFn/project template

@jyeshe
Copy link
Contributor Author

jyeshe commented Oct 18, 2023

@jyeshe , this is super cool, but i'd suggest it doesn't belong in Lightning. Would it make more sense in OpenFn/kit or OpenFn/adaptors? (My rational is this... if a developer is writing job state and expressions on their local filesystem, it's because they're using the CLI, not Lightning. What do you think? In other words, I love the concept but think it's in the wrong repo!)

Hey Taylor, thanks for clarifying. For this check I have looked at this: https://github.com/OpenFn/Lightning/blob/2b93dce6d7f057601a928658f28fd7600f001e91/lib/lightning_web/controllers/api/provisioning_controller.ex#L38
However it would happen only if someone accidentally dump it in the future to any file for debugging. No doubt this wouldn't be merged (: I will adapt this PR to the scope of dev tools and apply the check to the project template. Thanks

@jyeshe jyeshe changed the title Prevent git push with credentials Allows enabling hooks for code analysis Oct 18, 2023
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dd461d3) 89.31% compared to head (ff39211) 89.31%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1214   +/-   ##
=======================================
  Coverage   89.31%   89.31%           
=======================================
  Files         231      231           
  Lines        7227     7227           
=======================================
  Hits         6455     6455           
  Misses        772      772           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jyeshe jyeshe requested a review from stuartc November 30, 2023 10:16
@jyeshe jyeshe self-assigned this Dec 4, 2023
@NickOpenFn
Copy link

Blocked until @stuartc is back

@stuartc
Copy link
Member

stuartc commented Feb 9, 2024

@jyeshe @taylordowns2000 can I get an explanation of what these changes do, I mean I see the git hooks - but no changes to the README or really what the justification is.

I can figure it out but this does change things for anyone contributing to the project - for these kinds of things I feel it's fair to have a proposal as why it's a good change.

Aside:

   plt_add_apps: [:mix, :ex_unit],

Curious why :ex_unit needed to be addedd to the plt?

@jyeshe
Copy link
Contributor Author

jyeshe commented Feb 12, 2024

@jyeshe @taylordowns2000 can I get an explanation of what these changes do, I mean I see the git hooks - but no changes to the README or really what the justification is.

I can figure it out but this does change things for anyone contributing to the project - for these kinds of things I feel it's fair to have a proposal as why it's a good change.

Aside:

   plt_add_apps: [:mix, :ex_unit],

Curious why :ex_unit needed to be addedd to the plt?

Basically it avoids code to be committed without proper formatting. From time to time we need to push again because it was not formatted (making the diff more precise as well). On a git push it runs the mix credo which is also super fast on the project instead of waiting for the CI.

The CI only passed with the :ex_unit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Icebox
Development

Successfully merging this pull request may close these issues.

None yet

4 participants