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.

Sign up for GitHub

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

Jump to bottom

feat: Allow env vars expansion in --args section for all hooks #363

Merged
merged 11 commits into from
Apr 26, 2022
Merged

feat: Allow env vars expansion in --args section for all hooks #363

merged 11 commits into from
Apr 26, 2022

Conversation

MaxymVlasov
Copy link
Collaborator

@MaxymVlasov MaxymVlasov commented Apr 15, 2022 •
edited

* - All hooks, except deprecated.

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

Implement a use case from #347, but without affecting UX.

Use case:

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.

Close #347

How can we test changes

  1. Export env vars, eg export CONFIG_NAME=.tflint; export CONFIG_EXT=hcl

  2. Add env vars to your .pre-commit-config.yaml`

repos:
- repo: https://github.com/antonbabenko/pre-commit-terraform
  rev: 182254e43fdfa6ff79f849cbaeb150d9c593a383
  hooks:
    - id: terraform_tflint
      args:
        # - '--args=--config=__GIT_WORKING_DIR__/${CONFIG_FILE}' # export CONFIG_FILE=.tflint.hcl
        - --args=--config=__GIT_WORKING_DIR__/${CONFIG_NAME}.${CONFIG_EXT} # export CONFIG_NAME=.tflint; export CONFIG_EXT=hcl
        - --args=--module
  1. Make sure that tested hook will fail during the code issue
  2. Run pre-commit run

Output:
image

@MaxymVlasov MaxymVlasov added the feature New feature or request label Apr 15, 2022
yermulnik
yermulnik reviewed Apr 15, 2022
View reviewed changes
Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

Great quirk dude!
Please see my suggestions for shell code changes below. I haven't tested them though, hence I kindly ask you to do this in your test environment. Thanks!

hooks/_common.sh Outdated Show resolved Hide resolved
hooks/_common.sh Outdated Show resolved Hide resolved
hooks/_common.sh Outdated Show resolved Hide resolved
hooks/_common.sh Outdated Show resolved Hide resolved
hooks/_common.sh Outdated Show resolved Hide resolved
MaxymVlasov and others added 2 commits April 15, 2022 21:47
@MaxymVlasov @yermulnik
Apply suggestions from code review
d091687 
Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
@MaxymVlasov @yermulnik
Apply suggestions from code review
ed8b445 
Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
MaxymVlasov
MaxymVlasov commented Apr 15, 2022
View reviewed changes
hooks/_common.sh Outdated Show resolved Hide resolved
MaxymVlasov
MaxymVlasov commented Apr 15, 2022
View reviewed changes
hooks/_common.sh Outdated Show resolved Hide resolved
MaxymVlasov
MaxymVlasov commented Apr 15, 2022
View reviewed changes
hooks/_common.sh Outdated Show resolved Hide resolved
yermulnik
yermulnik approved these changes Apr 15, 2022
View reviewed changes
Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

Apart from cut -d ... workaround, everything else looks just marvelous and great!
Let's merge if %%\}* isn't doing what's intended.

MaxymVlasov
MaxymVlasov commented Apr 15, 2022
View reviewed changes
hooks/_common.sh Outdated Show resolved Hide resolved
@MaxymVlasov
Copy link
Collaborator Author

@antonbabenko please check the idea.
Do not merge - we waiting for @andrew-glenn feedback

yermulnik
yermulnik reviewed Apr 15, 2022
View reviewed changes
hooks/_common.sh Outdated Show resolved Hide resolved
hooks/_common.sh Outdated Show resolved Hide resolved
@MaxymVlasov MaxymVlasov self-assigned this Apr 16, 2022
@MaxymVlasov @yermulnik
Apply suggestions from code review
89551ad 
Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
MaxymVlasov
MaxymVlasov commented Apr 16, 2022
View reviewed changes
hooks/_common.sh Outdated Show resolved Hide resolved
yermulnik
yermulnik approved these changes Apr 16, 2022
View reviewed changes
hooks/_common.sh Outdated Show resolved Hide resolved
@MaxymVlasov
Copy link
Collaborator Author

@andrew-glenn ping

@antonbabenko antonbabenko changed the title feat: Allow env vars expansion in --args section for all hooks* feat: Allow env vars expansion in --args section for all hooks Apr 26, 2022
@antonbabenko antonbabenko merged commit caa01c3 into master Apr 26, 2022
@antonbabenko antonbabenko deleted the feat/support_env_vars_expansion branch April 26, 2022 10:34
antonbabenko pushed a commit that referenced this pull request Apr 26, 2022
@semantic-release-bot
chore(release): version 1.69.0 [skip ci]
b4d8fd9 
# [1.69.0](v1.68.1...v1.69.0) (2022-04-26)

### Features

* Allow env vars expansion in `--args` section for all hooks ([#363](#363)) ([caa01c3](caa01c3))
@antonbabenko
Copy link
Owner

This PR is included in version 1.69.0 🎉

@andrew-glenn
Copy link
Contributor

<3

@yermulnik yermulnik mentioned this pull request Apr 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yermulnik yermulnik yermulnik approved these changes

@antonbabenko antonbabenko antonbabenko approved these changes

Assignees

@MaxymVlasov MaxymVlasov

Labels
feature New feature or request
Projects
None yet
Milestone
Features implementing and cleanup wit...
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MaxymVlasov @antonbabenko @andrew-glenn @yermulnik