pre-commit-golang icon indicating copy to clipboard operation
pre-commit-golang copied to clipboard

Add support for setting environment variables

Open timuralp opened this issue 3 years ago • 6 comments

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).

timuralp avatar Feb 03 '22 08:02 timuralp

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

TekWizely avatar Feb 04 '22 22:02 TekWizely

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.

timuralp avatar Feb 05 '22 00:02 timuralp

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:env is 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 beyond env in 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

TekWizely avatar Feb 05 '22 01:02 TekWizely

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:env is 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 beyond env in 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 avatar Feb 05 '22 01:02 timuralp

@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 avatar Oct 14 '22 21:10 TekWizely

@TekWizely that's great -- thank you! I will try to test this over the weekend. Apologies for not following through on the prior PR.

timuralp avatar Oct 14 '22 21:10 timuralp

Superseded by #27

timuralp avatar Oct 20 '22 05:10 timuralp