Skip to content

Conversation

@tdurk93
Copy link
Contributor

@tdurk93 tdurk93 commented Jan 30, 2024

secureli-373

Migrates .pre-commit-config.yaml into the .secureli/ directory. This directory was previously ignored by git, so I moved its existing contents into .secureli/local/ and told git to only ignore that subfolder. Using a nonstandard path for .pre-commit-config.yaml will allow our tool to not destroy any existing configuration/hooks, and allows us to sidestep much of the complexity of attempting to merge our hooks with existing hooks.

There are a lot of repeated hardcoded strings in this PR. I'm inclined to not do too much refactoring/rework to clean that up, since I'd like to do some significant refactoring in the near future.

Open Questions

  • I opted to keep the file name the same, but I don't love having a hidden file in the hidden folder (feels excessively "hidden"). I'm open to suggestions of whether pre-commit-config.yaml is better than .pre-commit-config.yaml.
  • This will break existing installations of secureli, so we should probably think about adding an automatic migration path prior to merging this. When secureli is invoked in a repo with an existing secureli installation, the user is met with this prompt: seCureLI has not yet been installed, install now? [Y/n]:

Testing

  • Existing unit tests pass
  • Manually tested by running secureli init in a new repository, then validated the contents & paths of the generated files.

I'd like to add a source-controlled .pre-commit-config.yaml
into `.secureli/`. This requires `.secureli/` to not be added to
.gitignore, so I'm moving the existing contents there to a `local/`
subfolder which will be ignored.
Using a custom location for our pre-commit hooks allows us to
not destroy whatever existing configuration exists, opening the
door to interoperability with existing installations of pre-commit
and avoiding conflicts when e.g. we want to use a different set of hooks
(or different versions of hooks) that what was already configured.
@tdurk93 tdurk93 marked this pull request as ready for review February 10, 2024 01:48
@tdurk93
Copy link
Contributor Author

tdurk93 commented Feb 10, 2024

This PR still needs some unit tests but otherwise I think it's pretty much ready for review.

Copy link
Contributor

@isaac-heist-slalom isaac-heist-slalom left a comment

Choose a reason for hiding this comment

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

Looks good!

@JordoHeffernan JordoHeffernan merged commit 00401c5 into main Mar 8, 2024
@JordoHeffernan JordoHeffernan deleted the feat/secureli-373-use-custom-pre-commit-config branch March 8, 2024 19:57
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.

6 participants