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

feat: Adding getopt flag and feature: --env-passthrough #347

Closed
wants to merge 3 commits into from
Closed

feat: Adding getopt flag and feature: --env-passthrough #347

wants to merge 3 commits into from

Conversation

andrew-glenn
Copy link
Contributor

@andrew-glenn andrew-glenn commented Mar 11, 2022

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

Context: I need to pass in the location of an alternate tflint config in CI.
The location is based on an environment variable in my CI environment, so
hard-coding the value as an argument isn't tenable.

This PR adds the __ENV_*__ placeholder, and the argument --env-passthrough.
The idea is to replace the __ENV_FOO__ placeholder with the value of
environment variable ${FOO}.

Example config leveraging the changes in this PR:

repos:
  - repo: https://github.com/antonbabenko/pre-commit-terraform
    rev: v1.65.0
    hooks:
      - id: terraform_tflint
        args:
          - "--env-passthrough=FOO"
          - "--args=--config=__ENV_FOO__/.tflint.hcl"

Note: hooks which do not use the common argument parser in _common.sh were not modified to use this functionality

How can we test changes

  • From AWS Codebuild:
    pre-commit try-repo https://andrew-glenn/pc-sandbox env-debug

For transparency, here's the .pre-commit-hooks.yaml config in my sandbox repo
https://github.com/andrew-glenn/pc-sandbox/blob/abcdad5b40aee08484185451912b8ad191f80138/.pre-commit-hooks.yaml

as well as the script that echo's args passed to it:

https://github.com/andrew-glenn/pc-sandbox/blob/abcdad5b40aee08484185451912b8ad191f80138/env.sh#L1-L19

  • Others:

Modify your local .pre-commit-config file to add my sandbox repo, replacing
FOO with an environment variable name of your choice.

repos:
  - repo: https://github.com/andrew-glenn/pc-sandbox
    rev: abcdad5b40aee08484185451912b8ad191f80138
    hooks:
      - id: env-debug
        args:
          - "--env-passthrough=FOO"
          - "--args=--config=__ENV_FOO__"

Example output:

===============================================================================
[INFO] Initializing environment for https://github.com/andrew-glenn/pc-sandbox.
env debug................................................................Passed
- hook id: env-debug
- duration: 0.01s

--foo=/codebuild/output/src982431412/src

@andrew-glenn andrew-glenn changed the title Addind getopt flag and feature: --env-passthrough feat: Adding getopt flag and feature: --env-passthrough Mar 11, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
@MaxymVlasov MaxymVlasov self-requested a review April 13, 2022 20:53
@MaxymVlasov MaxymVlasov added the feature New feature or request label Apr 13, 2022
Copy link
Collaborator

@MaxymVlasov MaxymVlasov left a comment

Choose a reason for hiding this comment

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

I think we can introduce logic that covers more cases.

Firstly, we should avoid difficult-to-understand logic for users.
Secondly, it should be simple to understand and know how to use (UX).

Hooks are not run in environment isolation (that's where from your use case) so, the first idea - is just to use the POSIX standard of environment variables.

I spend some time with @yermulnik brainstorming how it can be done and created #363.
Please check if that will work for you.

P.S. Before I start working on #363, I quickly review your PR and leave some comments. No more need to implement suggestions in them, I keep them JFYI, hope it will be helpful :)

#
# Example usage:
# - FOO=one
# - __ENV_FOO__ becomes "one"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, provide func doc as it specified in Google Shell styleguide

# - __ENV_FOO__ becomes "one"

function common::passthrough_env_vars {
local var
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Pass ENV_PASSTHRU from outside, don't use global vars.
  2. local var declaration not needed at all

@MaxymVlasov MaxymVlasov marked this pull request as draft April 15, 2022 19:52
@antonbabenko
Copy link
Owner

This issue has been resolved in version 1.69.0 🎉

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

Successfully merging this pull request may close these issues.

3 participants