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

When writing pre-commit hook, merge with existing hook logic #372

Closed
tdurk93 opened this issue Jan 17, 2024 · 1 comment · Fixed by #410
Closed

When writing pre-commit hook, merge with existing hook logic #372

tdurk93 opened this issue Jan 17, 2024 · 1 comment · Fixed by #410
Assignees

Comments

@tdurk93
Copy link
Contributor

tdurk93 commented Jan 17, 2024

If any functionality exists in .git/hook/pre-commit prior to running secureli init, we should not remove the preexisting functionality. Instead, we should integrate logic from secureli with the existing pre-commit hook code.

Note that we will want a way to identify any logic added via seCureLI so that we can make changes to it (e.g. updates) without affecting non-secureli logic.

Here are a few ways this could be implemented:

  1. (Easier but less correct) we could back up the state of .git/hooks/pre-commit at secureli install time. Any time we need to make a change/update to the hook, we can append the new logic onto the backup file. This won't account for any changes manually made to the pre-commit file after secureli was installed.
  2. (Slightly more difficult) we could add some delimiter (e.g. #### BEGIN seCureLI ####/#### END seCureLI ####) to indicate sections of the pre-commit file managed by seCureLI.
  3. We could have a single line in the pre-commit file that executes a separate script managed by seCureLI. Then we could rewrite/regenerate this script for any updates, and avoid making any additional changes to the pre-commit hook.
@tdurk93
Copy link
Contributor Author

tdurk93 commented Jan 23, 2024

After a call to brainstorm ideas, this is the current proposal:

  1. Check if .git/hook/pre-commit exists. If so, backup to .git/hooks/pre-commit.backup.<datetime>
  2. Mention in the output that we backed up the file
  3. Write the new file .git/hooks/pre-commit
  4. Include at the top of the file: # Auto-generated by seCureLI

@kevin-orlando kevin-orlando moved this from Todo to In Progress in seCureLI Jan 24, 2024
@kevin-orlando kevin-orlando self-assigned this Jan 24, 2024
@kevin-orlando kevin-orlando moved this from In Progress to PR Review in seCureLI Jan 30, 2024
kevin-orlando added a commit that referenced this issue Jan 31, 2024
secureli-372

closes #372 

Handles overwriting existing pre-commit install in the user's repo on
new install

## Changes
* Adds logic to check if pre-commit hook exists and if so, creates a
backup of the current file and overwrites it with SeCureLI's pre-commit
content
* Print a warning to the user to let them know that the file is being
overwritten and that a backup is created

## Testing
Initialize SeCureLI on a git repo with an existing pre-commit file
<img width="677" alt="Screenshot 2024-01-30 at 2 01 42 PM"
src="https://github.com/slalombuild/secureli/assets/58826693/11240502-db9b-4b29-a3aa-87b400518638">

## Clean Code Checklist
<!-- This is here to support you. Some/most checkboxes may not apply to
your change -->
- [x] Meets acceptance criteria for issue
- [x] New logic is covered with automated tests
- [x] Appropriate exception handling added
- [x] Thoughtful logging included
- [ ] Documentation is updated
- [ ] Follow-up work is documented in TODOs
- [ ] TODOs have a ticket associated with them
- [x] No commented-out code included


<!--
Github-flavored markdown reference:
https://docs.github.com/en/get-started/writing-on-github
-->

---------

Co-authored-by: Tyler D <[email protected]>
@github-project-automation github-project-automation bot moved this from PR Review to Done in seCureLI Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants