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

Prepare frigate integration #543

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Conversation

jrottenberg
Copy link
Contributor

https://github.com/rapidsai/frigate#pre-commit-hook

Frigate is a very nifty tool << for automatically generating documentation for your Helm charts.>>

@asottile
Copy link
Member

asottile commented Aug 10, 2021

a few things:

  • the readme should use >> .pre-commit-config.yaml probably so it doesn't clobber existing usages
  • the hook is configured with always_run: true + require_serial: true -- this does not guarantee it only runs in one invocation which seems to be the intention. I think you were trying to not pass filenames because it does not consume them in which case you should use pass_filenames: false
  • the trailing / on the repo: example is a bit weird, I would remove that

@jrottenberg
Copy link
Contributor Author

ok in progress, https://github.com/rapidsai/frigate/pull/36/files let me know if I'm missing anything.

jacobtomlinson pushed a commit to rapidsai/frigate that referenced this pull request Aug 16, 2021
@jrottenberg
Copy link
Contributor Author

@asottile , should be all good now.

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit 649858c into pre-commit:master Aug 18, 2021
@jrottenberg
Copy link
Contributor Author

thanks a lot, is there a way to add a quick description and set the hook type to 'helm' ?

@asottile
Copy link
Member

@jrottenberg that's all derived from the description and types fields in the hook metadata itself

@jrottenberg
Copy link
Contributor Author

gotcha, I'll update and wait for the next cron run, thanks again !

@asottile
Copy link
Member

I had to revert this repo as you broke the build: 3ca6206

@jrottenberg
Copy link
Contributor Author

I see yeah, I can reproduce locally. Not sure what to do from it, the hook classification is derived from https://github.com/pre-commit/identify which in no case will auto detect helm files... and for good reason.

I can remove the type, or patch your build script to allow a fallback if identify cannot detect the type and rely on the 'suggested' value (from the .pre-commit-hooks.yaml type)

@asottile
Copy link
Member

your configuration is unusable with pre-commit itself -- identify types is how it does file matching

@jrottenberg
Copy link
Contributor Author

Ah yes of course I'm not using that in the pre-commit (run always regardless of the files changed), I wrongly assumed it was metadata for pre-commit.com.

I'll revert so and keep only the description...

Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants