pre-commit-golang
pre-commit-golang copied to clipboard
Add support for setting environment variables
Allow setting environment variables when calling the Go pre-commit hooks. For example, this allows setting GOOS when checking code targeted for a different platform (e.g. Linux vs OS X).
Greetings @timuralp and thank you for your interest in my project!
Soo, the real issue here is that Pre-Commit doesn't make it convenient to pass environment variables to hooks.
What I would prefer you do before we consider how we might hack this feature in is to create an official issue request for env support on the pre-commit project:
- https://github.com/pre-commit/pre-commit
Here a previous discussion I could find on the matter:
- https://github.com/pre-commit/pre-commit/issues/758
My feeling is that the hook configuration section should support an env: entry that is a map of name/value pairs to add to the environment when invoking the hook.
Note: The maintainer is going to say no, but you'll need to make a strong case anyway. As I see it, the case goes:
- Go supports defining GOOS and GOARCH variables, which effect which code files are considered during processing.
- The OS and ARCH values cannot be set via command line options - Go only accepts environment variables for these.
The maintainer will suggest that you configure a hook with an inline script entry that sets the variables and invokes the command you want ...
You'll need to note that, in the cases where you are using one of my mod or pkg hooks, then those hooks determine a set of directories to act on, based on the given file list (or full repo) and the equivalent logic cannot be easily added to an inline entry.
Please feel free to post the link to the issue here (or mention this issue in that one, which will create a link) and I can follow along and chime in.
I would love to see env support added to the pre-commit tool.
Good Luck !
-TW
Thank you for the prompt response! I also followed the multiple environment variable related reports on the pre-commit project and concluded that I am unlikely to persuade the project owners that supporting environment variables is a worthwhile endeavor. I opened an issue highlighting that it is currently impossible to use the Golang hooks out of the box when the target platform differs from where the hook is run (https://github.com/pre-commit/pre-commit/issues/2237). I suspect that the response will be to encourage users to set an environment variable. That's why I proposed the hacky workaround to plumb through environment variables via a special argument.
I'm open to other ideas, but don't see great alternatives today, unfortunately.
Well at least the reply (and rejection) didn't take long.
Grinding on this, It's important to me that such a feature would use argument names that are highly unlikely to conflict with any tools that might ever be used.
To that end, my idea of the option format would be:
--hook:env name=value
- I want this solution to feel like a general solution that any pre-commit hook might adopt. i.e let's try to set a standard.
--hook:envis not likely [to ever] conflict with any tools.- Using
--for a long arg is more standard, even if go's flag parsing specifically doesn't need it. i.e. in the case of this specific repo, these may be the only args with--. - The
--hook:creates a name space for passing out-of-band options to hooks. We may come up with other features beyondenvin the future.
I'm a little torn regarding if we should expect these args at the beginning of the arg list, or be okay with them intermixed with other args. I think the logic I use allows them to be intermixed but am not sure if its wise to support it.
Lemme know your thoughts on this, and thanks for trying !
-TW
Grinding on this, It's important to me that such a feature would use argument names that are highly unlikely to conflict with any tools that might ever be used.
To that end, my idea of the option format would be:
--hook:env name=value
- I want this solution to feel like a general solution that any pre-commit hook might adopt. i.e let's try to set a standard.
--hook:envis not likely [to ever] conflict with any tools.- Using
--for a long arg is more standard, even if go's flag parsing specifically doesn't need it. i.e. in the case of this specific repo, these may be the only args with--.- The
--hook:creates a name space for passing out-of-band options to hooks. We may come up with other features beyondenvin the future.
Agreed. I picked -env- for similar reasons of trying to avoid collisions. Let me consider how easy it will be to parse this format. I'm unsure what else may need to be passed in circumventing the existing pre-commit infrastructure, but carving out the namespace makes sense.
I'm a little torn regarding if we should expect these args at the beginning of the arg list, or be okay with them intermixed with other args. I think the logic I use allows them to be intermixed but am not sure if its wise to support it.
I think from the hook consumer standpoint, a restriction on placement of arguments may be awkward or confusing. To improve usability, I'd consider parsing the options in the same way as all the arguments are parsed at the moment.
I'll rework the PR to incorporate your suggestions and we can iterate on the format. Thanks for the quick follow up!
@timuralp You'll be glad to know I haven't forgotten about this feature.
I just opened #27 and would appreciate your'e help in testing it if you can?
-TW
@TekWizely that's great -- thank you! I will try to test this over the weekend. Apologies for not following through on the prior PR.
Superseded by #27