[MS] Fix crash #7353, and support bool and int outputs #13670
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
All tests are passing except TestAccAzureRMVirtualMachineExtension_importBasic, but that tests fails for us even without any changes. Is there a known issue with this test? |
tombuildsstuff
suggested changes
Apr 20, 2017
There was a problem hiding this comment.
Thanks for this contribution :)
I've left a few comments inline, but this is looking good. The tests pass too:
$ envchain azurerm make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMTemplateDe'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/20 10:43:26 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMTemplateDe -timeout 120m
=== RUN TestAccAzureRMTemplateDeployment_basic
--- PASS: TestAccAzureRMTemplateDeployment_basic (276.07s)
=== RUN TestAccAzureRMTemplateDeployment_disappears
--- PASS: TestAccAzureRMTemplateDeployment_disappears (272.96s)
=== RUN TestAccAzureRMTemplateDeployment_withParams
--- PASS: TestAccAzureRMTemplateDeployment_withParams (276.59s)
=== RUN TestAccAzureRMTemplateDeployment_withOutputs
--- PASS: TestAccAzureRMTemplateDeployment_withOutputs (280.15s)
=== RUN TestAccAzureRMTemplateDeployment_withError
--- PASS: TestAccAzureRMTemplateDeployment_withError (185.68s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/azurerm 1291.473s
Thanks!
| outputStringValue = "1" | ||
| } else { | ||
| return fmt.Errorf("Invalid value %s for boolean output %s", outputValue, key) | ||
| } |
There was a problem hiding this comment.
We should be able to use Go's built-in Boolean conversion method here:
boolValue, ok := outputValue.(bool)
if !ok {
return fmt.Errorf("[ERROR] Output was a Bool, but was not a Boolean Value: '%+v'", outputValue)
}
outputStringValue = strconv.FormatBool(boolValue)
There was a problem hiding this comment.
BTW, most of the code I see does not add [ERROR] to error messages, just to logging? What's the standard? Presumably [ERROR]'s not needed since it already shows up as an error?
There was a problem hiding this comment.
Regarding Booleans, the documentation in https://www.terraform.io/docs/configuration/variables.html is a little confusing, but it looked to me like I should be explicitly using "0" and "1" instead of "false" and "true". Is that correct? If I set an output in a .tf file to be based off of a Boolean variable, I get "0" and "1" values. (Also, as far as I can tell, there's no way to configure azurerm_template_deployment.outputs to be a map of interface{}, it has to be a consistent element value.)
There was a problem hiding this comment.
BTW, most of the code I see does not add [ERROR] to error messages, just to logging? What's the standard? Presumably [ERROR]'s not needed since it already shows up as an error?
Correct - apologies, I should have given a better example. It's needed on Printf statements, but Errors get it automatically
Regarding Booleans, the documentation in https://www.terraform.io/docs/configuration/variables.html is a little confusing, but it looked to me like I should be explicitly using "0" and "1" instead of "false" and "true". Is that correct? If I set an output in a .tf file to be based off of a Boolean variable, I get "0" and "1" values.
Usually, we'd just rely on the d.Set method to handle serializing these objects into State, and not handle that directly. However given these are having to be cast to a string - I think it makes the most sense to serialize them using Go's built in strconv.ParseBool() / strconv.FormatBool() methods, so we can switch it out easily in the future?
| outputType := outputMap["type"] | ||
| var outputStringValue string | ||
|
|
||
| switch outputType { |
There was a problem hiding this comment.
Given the Documentation shows the value for type can be in lower-case - it'd be great to make this check ignore the case, e.g.:
switch strings.ToLower(outputType) {
case "bool":
...
| outputMap := output.(map[string]interface{}) | ||
| outputValue, ok := outputMap["value"] | ||
| if !ok { | ||
| // No value | ||
| continue | ||
| } | ||
| outputType := outputMap["type"] |
There was a problem hiding this comment.
Can we check for the existence of the type field as we are for the value field above?
| case "Int": | ||
| outputStringValue = fmt.Sprint(outputValue) | ||
|
|
||
| case "SecureString", "Object", "SecureObject", "Array": |
There was a problem hiding this comment.
I think we can remove this Case statement seeing as the Type's aren't supported?
| case "SecureString", "Object", "SecureObject", "Array": | ||
| fallthrough | ||
| default: | ||
| log.Printf("[WARNING] Ignoring output %s: Outputs of type %s are not currently supported in azurerm_deployment_template.", |
There was a problem hiding this comment.
could we update this to use [WARN] rather than [WARNING]?
There was a problem hiding this comment.
Also, is there a way to actual print an actual warning in the main Terraform output (as opposed to just the debugging output)? I originally returned an error here, but that caused problems because the outputs still ended up in the state file and continued causing errors even after the user corrected the problem). Haven't investigated how to fix that and return an actual error.
There was a problem hiding this comment.
Also, is there a way to actual print an actual warning in the main Terraform output (as opposed to just the debugging output)? I originally returned an error here, but that caused problems because the outputs still ended up in the state file and continued causing errors even after the user corrected the problem). Haven't investigated how to fix that and return an actual error.
Unfortunately not from within a CRUD method - that's only possible during Schema Validation.
Potentially we could validate the Template via the ValidateFunc to ensure there's only Output's of supported types - however I believe a warning in the documentation should suffice for now?
| @@ -98,6 +98,7 @@ The following arguments are supported: | |||
| The following attributes are exported: | |||
|
|
|||
| * `id` - The Template Deployment ID. | |||
| * `outputs` - A map of supported scalar output types returned from the deployment (currently, Azure template outputs of type String, Int and Bool are supported, and are converted to strings - others will be ignored). | |||
There was a problem hiding this comment.
currently, Azure template outputs of type String, Int and Bool are supported, and are converted to strings - others will be ignored
Is it worth changing Azure template outputs -> AzureRM Template Deployment Outputs of.. for consistency here?
…t outputs Included in this fix: 1) No crash 2) Debug log indicates problem, otherwise unsupported outputs are ignored 3) String, bool and int outputs are supported 4) Documentation indicates these limitations What is not included: 5) Array, object, securestring, secureobject still not supported
| outputMap := output.(map[string]interface{}) | ||
| outputValue, ok := outputMap["value"] | ||
| if !ok { | ||
| // No value | ||
| continue | ||
| } | ||
| outputType := outputMap["type"] |
There was a problem hiding this comment.
Since these come straight from Azure, and it does its own type checks, I've removed those checks in my code.
|
@tombuildsstuff Fixes made, see comments for some questions. ==> Checking that code complies with gofmt requirements... |
|
Sorry for the delay getting back to this! I've replied to the questions in-line - the one thing outstanding is the Boolean serialisation; if we can switch it out for Thanks :) |
tombuildsstuff
added
the
waiting-response
|
@tombuildsstuff Okay, changed to str.FormatBool(). Thanks. ==> Checking that code complies with gofmt requirements... |
StephenWeatherford
changed the title
tombuildsstuff
removed
the
waiting-response
tombuildsstuff
approved these changes
May 3, 2017
|
Tests pass: |
tombuildsstuff
added a commit
that referenced
this pull request
May 3, 2017
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix crash for outputs of type Object (#7353), and support bool and int outputs
Included in this fix:
What is not included:
5) Array, object, securestring, secureobject still not supported