Fix some minor issues in Go codes templates by houkms · Pull Request #13 · VSChina/magic-modules

houkms · 2019-08-06T04:24:15Z

Fix some minor issues in Go codes templates.

JunyiYi

Thanks @houkms for the update. The code looks good except for some tiny naming and space issues. Let's use this PR to track most of the issues discovered in the Azure Frontdoor Firewall Policy resource.

JunyiYi · 2019-08-06T05:35:13Z

templates/azure/terraform/expand_property_method.erb

 <%        elsif property.is_a?(Api::Type::Array) -%>
    results := make([]<%= sdk_marshal.package -%>.<%= sdk_marshal.sdktype.go_type_name -%>, 0)
    for _, v := range input {
+      v := v.(map[string]interface{})


Suggested change

v := v.(map[string]interface{})

v := item.(map[string]interface{})

Should it be item := item.(map[string]interface{}) ?

The generated .go code is like this

No, it should be v := item.(map[string]interface{})

JunyiYi · 2019-08-06T05:35:22Z

templates/azure/terraform/expand_property_method.erb

@@ -74,6 +74,7 @@ func expand<%= sdk_marshal.resource -%><%= descriptor.func_name -%>(input <%= go
 <%        elsif property.is_a?(Api::Type::Array) -%>
    results := make([]<%= sdk_marshal.package -%>.<%= sdk_marshal.sdktype.go_type_name -%>, 0)
    for _, v := range input {


Suggested change

for _, v := range input {

for _, item := range input {

And rename the original item to result.

houkms · 2019-08-06T06:51:11Z

@JunyiYi Codes are modified, find your time to check it.

JunyiYi · 2019-08-06T19:19:32Z

Thanks @houkms for the update. Please use v := item.(map[string]interface{}) instead of item := ... in the template because we will use v later in build_schema_property_get('v', output_var, prop, sdk_marshal, 8).

houkms · 2019-08-07T01:52:38Z

OK, fixed.

houkms · 2019-08-07T03:33:56Z

This PR fix the issue in this magic-module-specs PR.

houkms · 2019-08-12T17:02:53Z

@JunyiYi Other possible issues are fixed in the above commit, including

Go SDK rename: FlattenStringArray --> FlattenStringSlice and ExpandStringArray --> ExpandStringSlice
String array decision condition adjustment
Terraform configuration grammer: use block for array
Introduce help function to flatten example properties

Please find your time to review.

houkms · 2019-08-12T17:06:35Z

This PR also fixes the pipeline error in the magic-module-specs PR102

JunyiYi

Thanks @houkms , after taking a look at the PR, I think most of the string concatenation or to_s stuffs could be simplified by Ruby String Interpolation.

JunyiYi · 2019-08-12T19:33:55Z

provider/azure/terraform/example/helpers.rb

+            properties.each do |pn, pv|
+              if pv.is_a?(Hash)
+                pv.each do |key, val|
+                  flat_properties[pn+"."+key] = val


You can simplify the string with "#{pn}.#{key}".

Replaced! Also done for other similar cases.

JunyiYi · 2019-08-12T19:35:34Z

provider/azure/terraform/example/helpers.rb

+            flatten_example_properties_to_check(example_props, true)
+          end
+
+          def flatten_example_properties_to_check(properties, is_nested)


Here is_nested, do you mean has_nested_item?

Yes, and the name was changed to be has_nested_item

JunyiYi · 2019-08-12T19:43:19Z

templates/azure/terraform/example/hcl_properties.erb

 <%      elsif prop[1].is_a?(Integer) || prop[1].is_a?(TrueClass) || prop[1].is_a?(FalseClass) -%>
 <%=       prop[0].ljust(prop_alignment) -%> = <%= lines(prop[1].to_s) -%>
+<%      elsif prop[1].is_a?(Array) && (prop[1].length == 0 || prop[1][0].is_a?(String))
+          res_arr = prop[1].collect{|elem| "\""+elem.to_s+"\""} -%>


Can you try the following code whether it works or not?

Suggested change

res_arr = prop[1].collect{|elem| "\""+elem.to_s+"\""} -%>

res_arr = prop[1].map{|elem| "\"#{elem}\""} -%>

It works, replaced now.

houkms · 2019-08-13T02:09:20Z

@JunyiYi Codes were refined now according to your suggestions.

JunyiYi

The code is pretty good to go except for one minor issue. We still need to keep the = for tags while omit it for other block properties.

JunyiYi · 2019-08-13T03:25:43Z

templates/azure/terraform/example/hcl_properties.erb

 -%>

-<%=       prop[0] -%> = {
+<%=       prop[0] -%> {


Please make sure we include the equal sign (=) for tags (You can define a new helper function in terraform/helpers.rb similar to this one for ansible). Here is the example in Terraform: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_eventgrid_topic_test.go#L201.

houkms · 2019-08-13T04:45:33Z

Added codes to deal with the special case for tags property.

JunyiYi · 2019-08-13T05:45:09Z

templates/azure/terraform/example/hcl_properties.erb

 -%>

-<%=       prop[0] -%> = {
+<%=       prop[0] -%> <%= "= " if is_tags?(prop)-%>{


Well, I think for this case, it is not a good choice to introduce that common helper function. Let's do it inline: <%= '= ' if prop[0] == 'tags' -%>.

JunyiYi

Code LGTM, let's merge that in.

fix some minor issues in Go codes templates

35b16c8

JunyiYi requested changes Aug 6, 2019

View reviewed changes

code refined

1f1ba3a

fix minor logic error

6860e6a

houkms mentioned this pull request Aug 7, 2019

[WIP] Introduce SoftwareUpdateConfiguration resource Azure/magic-module-specs#84

Open

fix redundant brace in template

c60c1c8

fix issues in code generation for terraform

d2c33a7

houkms mentioned this pull request Aug 12, 2019

Introduce WebApplicationFirewallPolicy resource Azure/magic-module-specs#102

Merged

JunyiYi requested changes Aug 12, 2019

View reviewed changes

code refined

69f8211

JunyiYi requested changes Aug 13, 2019

View reviewed changes

special case for tags property in exmaple

242115a

JunyiYi reviewed Aug 13, 2019

View reviewed changes

code refined

4b5a5b9

JunyiYi approved these changes Aug 13, 2019

View reviewed changes

JunyiYi merged commit cbf48d4 into VSChina:azure_backend Aug 13, 2019

	v := v.(map[string]interface{})
	v := item.(map[string]interface{})

	res_arr = prop[1].collect{\|elem\| "\""+elem.to_s+"\""} -%>
	res_arr = prop[1].map{\|elem\| "\"#{elem}\""} -%>

Comments

Conversation

houkms commented Aug 6, 2019

Uh oh!

JunyiYi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houkms commented Aug 6, 2019

Uh oh!

JunyiYi commented Aug 6, 2019

Uh oh!

houkms commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houkms commented Aug 7, 2019

Uh oh!

houkms commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houkms commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JunyiYi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houkms Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houkms commented Aug 13, 2019

Uh oh!

JunyiYi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houkms commented Aug 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunyiYi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

houkms commented Aug 7, 2019 •

edited

Loading

houkms commented Aug 12, 2019 •

edited

Loading

houkms commented Aug 12, 2019 •

edited

Loading

houkms Aug 13, 2019 •

edited

Loading