Skip to content

Add githook script to alert on changes that require re-installs #6757

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dominikschubert
Copy link
Member

Uses pre-commit to register a local python script that will be executed after running git checkout, i.e. when switching branches. It contains a list of files and if the diff between the two commits includes a change in these files, it will output a warning.

How to use:
Run make init-githooks to set up the hooks for your current repository.
You can trigger the behavior by switching to any branch with some change in any of the files that are listed in the script.

Some things that I think should still be included before an initial merge:

  • warn on rebase
  • warn on pull
  • check changes in providers
  • check changes in plugins

References:

@dominikschubert dominikschubert self-assigned this Aug 25, 2022
@dominikschubert
Copy link
Member Author

@thrau @alexrashed

Would love some input on how to best check for provider/plugin changes. Ideally a minimal set that most likely requires at least a new make entrypoints.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.422% when pulling d790fdc on githook_branchswitch_depscheck into 44a7f05 on master.

@alexrashed
Copy link
Member

The vast majority of entrypoints are registered in the providers.py.
Besides that, a lot of the hooks are registered in the different plugins.py (and this is definitely becoming a pattern for services as well, i.e. having a plugins.py next to the provider.py registering a set of service-specific hooks).
So, catching changes for files with these names would definitely be a good first heuristic if you want to proceed with the git diff-based approach.
An alternative would be to directly re-generate the entrypoints on every branch switch, but it takes a few seconds (for me ~6 seconds in community).

@dominikschubert dominikschubert added this to the Playground milestone Jul 12, 2023
@dominikschubert
Copy link
Member Author

@alexrashed what's your take on it, should we continue here or just close if for now? Haven't seen any push in that direction so I'd say it's not super urgent

@alexrashed
Copy link
Member

That's hard to say. I think the entrypoints still cause some confusion, but if we want to solve it, we should make sure that the solution catches all these issues (in order to avoid even more confusion).
But a hook supporting context switches and updates will become more important when we pin and regularly update our dependencies. We would like to support everyone in updating their envs to updated pinned deps there as well.
So imho:

  • I think the plugin system is used in a lot of places where a file based approach will be problematic, and I think tackling 90% of the issues makes it less obvious for the remaining 10%...
  • This approach will be perfect to remind users to update their venv when new requirements are pinned in the future!

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.

3 participants