Sitelet https://web.archive.org/web/20201130225041im_/https://github.com/pre-commit/pre-commit/issues/1352
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

Support whitelisting hooks installed when using pre-commit init-templatedir #1352

Open
deric4 opened this issue Mar 6, 2020 · 8 comments
Open
Labels

Comments

@deric4
Copy link

@deric4 deric4 commented Mar 6, 2020

Auto installing hooks has been one of my favorite features especially when bouncin around 100s of organization/personal repos but I'm also cloning quite a few "untrusted" repos. Having an easier way of whitelisting either individual hooks, hooks in a specific repos' .pre-commit-config.yaml, or even file paths would be would be pretty awesome, e.g ~/projects/trusted-repos/*.

Willing to take a crack at a PR if this is even doable. Either way, thanks for one of my favorite tools! :)

@asottile
Copy link
Member

@asottile asottile commented Mar 6, 2020

I'm not quite sure how this would work, the init-templatedir .git/hooks/* file is identical to the one from pre-commit install 🤔

@asottile asottile added the question label Mar 6, 2020
@asottile
Copy link
Member

@asottile asottile commented Apr 22, 2020

@deric4 did you have any additional ideas on how this would work?

@deric4
Copy link
Author

@deric4 deric4 commented Apr 22, 2020

I think so, I had gotten started on testing it out but got sidetracked 😬 but left off trying to tease apart pre_commit.store and see if I could come up with anything sane.

@deric4
Copy link
Author

@deric4 deric4 commented Apr 25, 2020

Was able to play with this some more today after finally getting vscode, homebrew, pyenv, and tox to play nice 🤕 .

I'm sure there's a fair bit that I haven't taken into account, but was hoping something along either of the two options below might not too far into the weeds 🙃 .

Configuration

Config file should support a list of allowed repos and/or hook ids. I don't know what a realistic schema for the config would be, or where/how pre-commit would source it quite yet, but something like

repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
     hooks:
       - check-docstring-first

 #maybe something like this for option 2?
 - src: https://github.com/pre-commit/*

Implementation

Option 1 - pre_commit:repository:_cloned_repository_hooks

At some point before Hook.create(...) in the return, pop any repos/hooks out of repo_config that aren't in the whitelist configuration:

def _cloned_repository_hooks(
repo_config: Dict[str, Any],
store: Store,
root_config: Dict[str, Any],
) -> Tuple[Hook, ...]:
repo, rev = repo_config['repo'], repo_config['rev']
manifest_path = os.path.join(store.clone(repo, rev), C.MANIFEST_FILE)
by_id = {hook['id']: hook for hook in load_manifest(manifest_path)}
for hook in repo_config['hooks']:
if hook['id'] not in by_id:
logger.error(
f'`{hook["id"]}` is not present in repository {repo}. '
f'Typo? Perhaps it is introduced in a newer version? '
f'Often `pre-commit autoupdate` fixes this.',
)
exit(1)
hook_dcts = [
_hook(by_id[hook['id']], hook, root_config=root_config)
for hook in repo_config['hooks']
]
return tuple(
Hook.create(
repo_config['repo'],
Prefix(store.clone(repo, rev, hook['additional_dependencies'])),
hook,
)
for hook in hook_dcts
)

debugging screen shot showing available vars/values at this point

pre-commit-debug-1

Option 2 - add additional config/repo info in ~/.cache/pre-commit/db.db

My initial proposal for supporting file path based whitelisting conflated my personal development patterns/layout and what I was really wanting: allow any hooks defined in repos from a whitelist of GitHub users or organizations.

example:

# whitelist config
repos:
  - src: https://github.com/asottile/*

Would allow the auto install of hooks defined in repositories under github/asottile

So cloning https://github.com/asottile/pyupgrade auto installs everything in that projects .pre-commit-config.yaml

https://github.com/asottile/pyupgrade/blob/771453a0bcf4714802edf9c0494537a2a367f789/.pre-commit-config.yaml#L1-L13

Not sure implementation would be very straightforward since the hook id, and which config files they're defined in, doesn't appear in db.db

dump of db.db after cloning pyupgrade

sqlite> select * from configs;
path
----------------------------------------------------------------------------
/Users/miguel/Projects/github.com/asottile/pyupgrade/.pre-commit-config.yaml
sqlite> select * from repos;
repo                                            ref         path
----------------------------------------------  ----------  --------------------------------------------
https://github.com/pre-commit/pre-commit-hooks  v2.5.0      /Users/miguel/.cache/pre-commit/repothnpq00z
https://github.com/asottile/setup-cfg-fmt       v1.6.0      /Users/miguel/.cache/pre-commit/reporzhan0yr
https://gitlab.com/pycqa/flake8                 3.7.9       /Users/miguel/.cache/pre-commit/repokws5qytb
https://gitlab.com/pycqa/flake8:flake8-typing-  3.7.9       /Users/miguel/.cache/pre-commit/repoo4gmop5n
https://github.com/pre-commit/mirrors-autopep8  v1.5        /Users/miguel/.cache/pre-commit/repoz2o_t7so
https://github.com/asottile/reorder_python_imp  v1.9.0      /Users/miguel/.cache/pre-commit/repo6hnue8ij
https://github.com/asottile/add-trailing-comma  v1.5.0      /Users/miguel/.cache/pre-commit/repojf1q9g12
https://github.com/asottile/pyupgrade           v2.3.0      /Users/miguel/.cache/pre-commit/repoeqlvaoud
https://github.com/pre-commit/mirrors-mypy      v0.770      /Users/miguel/.cache/pre-commit/repok8mxqvkq

Initially thought maybe some tom-foolery in pre_commit:store:Store init might make sense, along with a query that returns only whitelisted repos/hook ids:

class Store:
get_default_directory = staticmethod(_get_default_directory)
def __init__(self, directory: Optional[str] = None) -> None:

@asottile
Copy link
Member

@asottile asottile commented Apr 26, 2020

  • I'd rather it just error than silently slice out missing things
  • changing the database schema is pretty tough to do since it requires:
    • clever code for forward and backward compatibility
    • ensuring install layout stays the same

ideally, let's make this as broad as possible and only make it super granular if needed. I suspect that this means just whitelist of particular repositories (and maybe an extension to allow wildcards there? idk)

mostly just looking to keep the scope and complexity of this as small as possible -- don't really want to have to build something for like (repo, version, hook, language, language_version, additional_dependencies) at a particular value (since each of those can technically influence how a repository gets built). I also think that that's granular enough to support all the particular cases (if we're looking at "safety" vectors here, vetting a particular sub-hook of a repository isn't particularly valuable as an already-vetted hook changes over time).

as for config, maybe just a flat file? though that kinda closes the door on expanding this feature (though I can't really perceive any valuable expansions 🤔)

@deric4
Copy link
Author

@deric4 deric4 commented Apr 26, 2020

I'd rather it just error than silently slice out missing things

Makes sense. Would being able to ignore the white list configuration need to be supported (similar to git commit --no-verify orSKIP= )?

changing the database schema is pretty tough to do...
Ok, it seemed like a fairly large undertaking after diving in again today and wasn't sure if it truly would be or just my being unfamiliar with the code base.

ideally, let's make this as broad as possible and only make it super granular if needed. I suspect that this means just whitelist of particular repositories (and maybe an extension to allow wildcards there? idk)

White listing specific repos I think gets most of the way there, perhaps just a regex match against a list of allowed values?

as for config, maybe just a flat file?
I'm cool with that, I assumed that anything supported by the existing load_config would be easy enough.

load_config = functools.partial(
cfgv.load_from_filename,
schema=CONFIG_SCHEMA,
load_strategy=ordered_load_normalize_legacy_config,
exc_tp=InvalidConfigError,
)

Any recommendation on where to persist the config file? I had initially thought keeping it in ~/.cache/pre-commit but found out thats prob not a good idea since pre-commit clean completely wipes it out.

@asottile
Copy link
Member

@asottile asottile commented Apr 26, 2020

Makes sense. Would being able to ignore the white list configuration need to be supported (similar to git commit --no-verify orSKIP= )?

nah, probably the error message would just say "you configured this, you asked for this -- delete $CONFIG_FILE or add blah blah to it"

I'm cool with that, I assumed that anything supported by the existing load_config would be easy enough.

load_config is very specific to the .pre-commit-config.yaml format (load_manifest for example is a completely separate format)

Any recommendation on where to persist the config file? I had initially thought keeping it in ~/.cache/pre-commit but found out thats prob not a good idea since pre-commit clean completely wipes it out.

I don't think it would be "persisted" per-se, it would be edited by a human -- probably in ~/.config/pre-commit/...

@deric4
Copy link
Author

@deric4 deric4 commented Apr 26, 2020

Ok awesome, i'll try and take a crack at a PR. Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.