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

fix: Improve README and drop quotes from hook env vars #651

Merged
merged 3 commits into from Mar 25, 2024
Merged

fix: Improve README and drop quotes from hook env vars #651

merged 3 commits into from Mar 25, 2024

Conversation

yermulnik
Copy link
Collaborator

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

Fixes #650

  • Improve README by replacing sensitive AWS_ creds vars with AWS_PROFILE
  • Drop enclosing double quote from hook env vars

Kudos to @ericfrederich for spotting and reporting that

@yermulnik yermulnik self-assigned this Mar 22, 2024
@yermulnik yermulnik requested review from MaxymVlasov and removed request for MaxymVlasov March 22, 2024 23:40
@yermulnik yermulnik mentioned this pull request Mar 22, 2024
Closed
@ericfrederich
Copy link

Wow, thanks for the quick PR.

I'd prefer not to change functionality and to just change the README instead of adding code to implicitly strip quotes.
This functionality obviously hasn't been there/needed before, so why introduce it?

Instead just add documentation around the existing behavior... Just say everything before the = is the variable name and everything after is the value.

@yermulnik
Copy link
Collaborator Author

This functionality obviously hasn't been there/needed before, so why introduce it?

As you've found, the existing code adds extra quotes (or to be precise doesn't remove them whereas it apparently should be doing so). So why would we want to keep them? Instead we'd prefer to be consistent with how the shell exports vars.
The change should be rather a no-op for existing hooks, since, as you've found too, these extra enclosing quotes are not affecting variable handling by shell.

@yermulnik
Copy link
Collaborator Author

Re the changing the README, I don't think we should be dropping quotes from there as they serve to prevent word splitting and to distinguish (to delimit) the content of the inner (nested) var passed to the hooks as an env var.

@ericfrederich
Copy link

There is no word splitting as it's coming from yaml. It'll always be a single string.
It's not being interpreted by Bash, it's simply being split on the = sign. Left is the name, right is the value.
If you want to have it behave like Bash, where do you stop?... What else should be supported?
If it's anything other than quotes that will make it harder to support in Python.

@yermulnik
Copy link
Collaborator Author

If you want to have it behave like Bash, where do you stop?... What else should be supported?

I'm not following your point.
When shell (not specifically Bash) exports var, it does not put double quotes around the value.
You've found that "bug", I opened PR to fix it.
There's nothing else about this PR (plus the change to README to drop insecure suggestion).

@ericfrederich
Copy link

To me it's unclear what is supposed to process that line. Is it re-interpreted as Bash?... I would argue that it shouldn't.

Does it also support single quotes or just double? Does it support escape characters? Does it support $variables, what about ${variables}? What about commands like $(pwd)?

Even if you decide to only support double quotes, what if a value needs to contain a double quote... How are they to be escaped?

What if it starts with a quote but doesn't contain a closing quote. Is that an error?

I just need the behavior completely specified so I can move forward with a Python implementation for #648

For me it'd just be easier to choose some separation character, like = or : and say everything before the first occurrence of the separation char is name, everything after is value.

@yermulnik
Copy link
Collaborator Author

yermulnik commented Mar 25, 2024 •
edited

@ericfrederich Please move the discourse and the discussion to #648
This PR has nothing to do with your efforts in scope of #648 (apart from you being the one who spotted excessive quotes exported into env).

You can find most of the answers for the most of your questions in man 1 bash and in supporting documentation (help export and alike). From my standpoint, if you're willing to try and re-implement shell-based (mostly Bash-based) pre-commit hooks for Terraform in Python, then you defo need to get to know the "opponent" better.

ps: continuation of discourse in this PR will result in the conversation being locked to contributors only. Thanks for your understanding 🤝

@ericfrederich ericfrederich mentioned this pull request Mar 25, 2024
Open
MaxymVlasov
MaxymVlasov reviewed Mar 25, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
hooks/_common.sh Show resolved Hide resolved
README.md
Config example:

```yaml
- id: terraform_validate
args:
- --env-vars=AWS_DEFAULT_REGION="us-west-2"
- --env-vars=AWS_ACCESS_KEY_ID="anaccesskey"
- --env-vars=AWS_SECRET_ACCESS_KEY="asecretkey"
- --env-vars=AWS_PROFILE="my-aws-cli-profile"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, I found out why that functional was implemented - #116

No idea if is it worth highlighting such hack option in docs examples :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The options is nice I reckon. And the PR was not about it but to remove insecure example =)

@MaxymVlasov MaxymVlasov added the bug_with_workaround Something isn't working but there is a workaround label Mar 25, 2024
@yermulnik
Copy link
Collaborator Author

Oops, requested re-review because didn't notice the approve was already provided =)

@yermulnik yermulnik merged commit daec682 into master Mar 25, 2024
6 checks passed
@yermulnik yermulnik deleted the fix/Improve_README_and_drop_qoutes_from_hook_env_vars branch March 25, 2024 20:26
antonbabenko pushed a commit that referenced this pull request Mar 25, 2024
@semantic-release-bot
chore(release): version 1.88.4 [skip ci]
d9addbe 
## [1.88.4](v1.88.3...v1.88.4) (2024-03-25)

### Bug Fixes

* Improve README and drop quotes from hook env vars ([#651](#651)) ([daec682](daec682))
@antonbabenko
Copy link
Owner

This PR is included in version 1.88.4 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxymVlasov MaxymVlasov MaxymVlasov approved these changes

Assignees

@yermulnik yermulnik

Labels
bug_with_workaround Something isn't working but there is a workaround
Projects
None yet
Milestone
Features implementing and cleanup wit...
Development

Successfully merging this pull request may close these issues.

Quotation Marks in Environment Variables
4 participants
@yermulnik @ericfrederich @antonbabenko @MaxymVlasov