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: Add support for version constraints in tfupdate #437

Merged
merged 17 commits into from
Oct 6, 2022

Conversation

MaxymVlasov
Copy link
Collaborator

@MaxymVlasov MaxymVlasov commented Oct 4, 2022

Replace for #436 ?
Close #436 #388

  • shellcheck violation
  • fix other hooks, by adding "${#ARGS[@]}" and change per_dir_hook_unique_part args
  • fix pre-commit --all
  • fix the PASSED case, when args in the wrong order
      args:
        - --args=--version '~> 4.2.0'
        - --args=provider aws

@MaxymVlasov MaxymVlasov force-pushed the feat/GH-388/tfupdate_float_version_right_args branch from 92195ea to 35ca29e Compare October 4, 2022 15:55
@MaxymVlasov MaxymVlasov force-pushed the feat/GH-388/tfupdate_float_version_right_args branch from 23f9eee to dd470ad Compare October 4, 2022 16:00
hooks/_common.sh Outdated Show resolved Hide resolved
hooks/_common.sh Outdated Show resolved Hide resolved
declare -a args=()
# Expand args to a true array.
# Based on https://stackoverflow.com/a/10953834
while ((args_array_length-- > 0)); do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would a simpler code work here? This also makes no need for args_array_length var.

Suggested change
while ((args_array_length-- > 0)); do
while [[ $@ ]]; do

On the other hand the whole loop looks quite like simple args=("$@") 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I seem to think I understand why args_array_length is needed.
Though given common::per_dir_hook "$HOOK_ID" "${ARGS[*]}" "${FILES[@]}", which is used across this PR, the 2nd ARG to this current function has ARGS array rather than number of its elements — is this expected? So it's either common::per_dir_hook "$HOOK_ID" "${#ARGS[*]}" "${ARGS[*]}" "${FILES[@]}", or I am missing something essential 🤔 (probably it's right about time to leave this for tomorrow 😛)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I couldn't leave it for tomorrow.
This while thing doesn't work as expected for me, and given common::per_dir_hook "$HOOK_ID" "${ARGS[*]}" "${FILES[@]}" this should be that all same thing from our discussion in Slack:

  while read -r -d '' ARG; do
    args+=("$ARG")
  done < <(echo "$1" | xargs printf '%s\0')
  shift

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea why, but the suggested solution does not work here 🤔

7866b64 (#437)

I try to replace common::per_dir_hook "${ARGS[*]}" with common::per_dir_hook "${ARGS[@]}" but got a weird error

dirname: unrecognized option '--version__REPLACED__SPACE__">__REPLACED__SPACE__1.1.8"'
Try 'dirname --help' for more information.

Also, no luck with changing per_dir_hook_unique_part "${args[@]}" to per_dir_hook_unique_part "${args[*]}"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm-m-m, that's odd. By the way how come args come in with __REPLACED__SPACE__ thing in them? Is the __REPLACED__SPACE__ to be replaced with actual space later down the code? 🤔
I didn't check the code with __REPLACED__SPACE__ in args instead of spaces...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the REPLACED__SPACE to be replaced with actual space later down the code?

Should be. Firstly, it replaces spaces with placeholders and then placeholders with spaces. Check file for __REPLACED__SPACE__

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah, I mean is this expected that at this place the __REPLACED__SPACE__ isn't replaced with spaces yet? This may influence how we should be parsing array elements since elements values are not space-separated string, but __REPLACED__SPACE__-separated string.

hooks/tfupdate.sh Outdated Show resolved Hide resolved
hooks/tfupdate.sh Outdated Show resolved Hide resolved
hooks/tfupdate.sh Outdated Show resolved Hide resolved
hooks/tfupdate.sh Outdated Show resolved Hide resolved
hooks/tfupdate.sh Outdated Show resolved Hide resolved
declare -a args=()
# Expand args to a true array.
# Based on https://stackoverflow.com/a/10953834
while ((args_array_length-- > 0)); do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I seem to think I understand why args_array_length is needed.
Though given common::per_dir_hook "$HOOK_ID" "${ARGS[*]}" "${FILES[@]}", which is used across this PR, the 2nd ARG to this current function has ARGS array rather than number of its elements — is this expected? So it's either common::per_dir_hook "$HOOK_ID" "${#ARGS[*]}" "${ARGS[*]}" "${FILES[@]}", or I am missing something essential 🤔 (probably it's right about time to leave this for tomorrow 😛)

declare -a args=()
# Expand args to a true array.
# Based on https://stackoverflow.com/a/10953834
while ((args_array_length-- > 0)); do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I couldn't leave it for tomorrow.
This while thing doesn't work as expected for me, and given common::per_dir_hook "$HOOK_ID" "${ARGS[*]}" "${FILES[@]}" this should be that all same thing from our discussion in Slack:

  while read -r -d '' ARG; do
    args+=("$ARG")
  done < <(echo "$1" | xargs printf '%s\0')
  shift

hooks/terraform_tfsec.sh Show resolved Hide resolved
hooks/_common.sh Outdated Show resolved Hide resolved
@MaxymVlasov MaxymVlasov changed the title Feat/gh 388/tfupdate float version right args feat: Add support for version constraints in tfupdate Oct 6, 2022
@MaxymVlasov MaxymVlasov marked this pull request as ready for review October 6, 2022 13:54
hooks/terraform_docs.sh Outdated Show resolved Hide resolved
hooks/terraform_docs.sh Outdated Show resolved Hide resolved
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.

Black magic 🤣
Great job hardworking on this, Max. Thank you! 👍🏻

@antonbabenko antonbabenko merged commit a446642 into master Oct 6, 2022
@antonbabenko antonbabenko deleted the feat/GH-388/tfupdate_float_version_right_args branch October 6, 2022 16:16
antonbabenko pushed a commit that referenced this pull request Oct 6, 2022
# [1.76.0](v1.75.0...v1.76.0) (2022-10-06)

### Features

* Add support for version constraints in `tfupdate` ([#437](#437)) ([a446642](a446642))
@antonbabenko
Copy link
Owner

This PR is included in version 1.76.0 🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants