-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix(terraform.go): remove trailing newline from output value #50
Conversation
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: Vaughn Dice <vadice@microsoft.com>
907f716
to
bb29cc8
Compare
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@carolynvs I added a check to the current cli tests but have also created an issue around your suggestion to look into possibilities for unit test coverage in the future: #51 |
scripts/test/test-cli.sh
Outdated
exit 1 | ||
fi | ||
|
||
# Verify the output has no extra newline (mixin should trim newline added by terraform cli) |
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 extra check here necessary now that we are doing the exact equality check above? If there was a newline, wouldn't the != "bar"
catch it?
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.
Great question. Alas, bash's default command substitution strips trailing newline characters. I've been looking into ways to work around this (https://stackoverflow.com/questions/15184358/how-to-avoid-bash-command-substitution-to-remove-the-newline-character has been particularly helpful), but they seem to quickly grow ungainly. I also see that I should create a function to validate a given output, as we do so after install as well; so I'll at least update to DRY that up...
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
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.
LGTM, thanks!
terraform output <output name>
prior to bubbling upFixes #20
Note: Although the
--json
format flag onterraform output
looks handy (as noted in #20 (comment)) it appears the terraform CLI appends the newline character in this case as well -- and we'd have the extra task of removing the wrapping double quotes. Hence, I went the simpler route and stuck with the default format.