-
-
Notifications
You must be signed in to change notification settings - Fork 540
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
feat: Add support for version constraints in tfupdate
#437
Conversation
92195ea
to
35ca29e
Compare
23f9eee
to
dd470ad
Compare
declare -a args=() | ||
# Expand args to a true array. | ||
# Based on https://stackoverflow.com/a/10953834 | ||
while ((args_array_length-- > 0)); do |
There was a problem hiding this comment.
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.
while ((args_array_length-- > 0)); do | |
while [[ $@ ]]; do |
On the other hand the whole loop looks quite like simple args=("$@")
🤔
There was a problem hiding this comment.
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 😛)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤔
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[*]}"
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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__
There was a problem hiding this comment.
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.
declare -a args=() | ||
# Expand args to a true array. | ||
# Based on https://stackoverflow.com/a/10953834 | ||
while ((args_array_length-- > 0)); do |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>
This reverts commit 7866b64.
tfupdate
There was a problem hiding this 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! 👍🏻
# [1.76.0](v1.75.0...v1.76.0) (2022-10-06) ### Features * Add support for version constraints in `tfupdate` ([#437](#437)) ([a446642](a446642))
This PR is included in version 1.76.0 🎉 |
Replace for #436 ?
Close #436 #388
"${#ARGS[@]}"
and changeper_dir_hook_unique_part
args