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

Changes to pre commit hook. Fixes #794 #795

Merged
merged 6 commits into from Feb 22, 2023

Conversation

Jamie-SA
Copy link
Contributor

@Jamie-SA Jamie-SA commented Feb 22, 2023

Fixes #794

  • Fix serialize pre-commit hook
    • Make sure the script starts in the root directory of the repository
    • Correct the path to serializer
    • Add file to the commit after the sed command
    • Preserve file permissions during processing steps
    • Return a failure code (non zero return value) when the serializer fails
  • Add new "root" pre-commit hook
    • Prevent commits to these branches: develop, main, master
    • Then run the serializer pre-commit hook
  • Add a setup.sh file to install the pre-commit hook
  • Update the documentation a little

Everyone should run the script ./tools/setup.sh after pulling in these changes.

Jamie-SA added 5 commits February 21, 2023 18:33
Add new hook to prevent commits to protected branches.
Fix serializer hook to correctly find serializer jar file.
@rjyounes
Copy link
Collaborator

@Jamie-SA We have branch protection rules on develop and main. Why do we need to replicate them in the pre-commit hook?

@Jamie-SA
Copy link
Contributor Author

Jamie-SA commented Feb 22, 2023

@Jamie-SA We have branch protection rules on develop and main. Why do we need to replicate them in the pre-commit hook?

Because it prevents someone from doing what you did yesterday. It stops you from making the commit to the branch in the first place, as opposed to stopping you from pushing the branch to github. I personally much prefer to be told up front that I forgot to create a feature branch, instead of having to fix the problem afterwards.

@rjyounes
Copy link
Collaborator

OK, makes sense. I love the fact that my stupidity was the driver of an improvement. :)

@Jamie-SA
Copy link
Contributor Author

Well, I've done it several times myself which is why I already had done this for the dca repo and figured it would be easy to add to gist.

Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

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

Everything looks good here, and I've tested it out.

@rjyounes rjyounes merged commit 9296cc5 into develop Feb 22, 2023
@rjyounes
Copy link
Collaborator

@Jamie-SA Hm, it just occurred to me - do .sh scripts run on Windows? Do they have to be using a bash shell rather than the Windows PowerShell?

@Jamie-SA
Copy link
Contributor Author

Good point. I suppose we could just update the docs for now and tell people on Windows to copy the pre-commit file, or we could write the equivalent .bat file. I went with the script because we may want to add other things to it in the future and it is just easier for people that are not that good with a command line.

@Jamie-SA
Copy link
Contributor Author

I looked into it, there might be a way to write a script that will work on both. I will see if I can get it to work.

@rjyounes
Copy link
Collaborator

We could also tell windows users they can run the sh file in a bash shell - which people will have if they use VSCode, for example. But if you can write a single script that would be fine. Two is not ideal, obviously, for maintenance reasons.

You need to open a new PR since I already merged this one.

@rjyounes
Copy link
Collaborator

@Jamie-SA Added issue #797 to track this change.

@rjyounes rjyounes deleted the feature-add-check-branch-to-pre-commit-hook branch June 29, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pre-commit hook not running
2 participants