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.

Sign up for GitHub

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

Jump to bottom

Marked customer_name field as optional for interconnect resource #11640

Merged
merged 10 commits into from
Sep 24, 2024
Merged

Marked customer_name field as optional for interconnect resource #11640

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0d61f5c
Marked customer_name field as optional
abhijeetkjha-google Sep 5, 2024
616214f
removed customer_name field from the example
abhijeetkjha-google Sep 6, 2024
c4fa4cc
Added create test for cross cloud interconnect
abhijeetkjha-google Sep 13, 2024
ac24639
Added example
abhijeetkjha-google Sep 13, 2024
55f7d64
Merge branch 'GoogleCloudPlatform:main' into bugFix-361094483
abhijeetkjha-google Sep 19, 2024
d0b54e4
removed create test as no testing location is provided yet
abhijeetkjha-google Sep 19, 2024
73e8056
lint issues
abhijeetkjha-google Sep 19, 2024
0a2a7f5
Updated description for customerName field
abhijeetkjha-google Sep 23, 2024
77cd76a
Updated description for customerName field - 2
abhijeetkjha-google Sep 23, 2024
fae33ec
Updated description for location field
abhijeetkjha-google Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions mmv1/products/compute/Interconnect.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ examples:
primary_resource_id: 'example-interconnect'
vars:
interconnect_name: 'example-interconnect'
customer_name: 'example_customer'
deletion_protection: 'true'
test_vars_overrides:
deletion_protection: 'false'
Expand Down Expand Up @@ -145,7 +144,6 @@ properties:
Customer name, to put in the Letter of Authorization as the party authorized to request a
crossconnect.
immutable: true
required: true
Copy link
Member

Choose a reason for hiding this comment

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

Could you please update the description of this field to clarify when this field is required and when it is not? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

- !ruby/object:Api::Type::Enum
name: 'operationalStatus'
description: |
Expand Down
1 change: 0 additions & 1 deletion mmv1/templates/terraform/examples/compute_interconnect_basic.tf.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ data "google_project" "project" {}

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this example is skipped from test generation. It will update the examples in the web documentation though. Would you mind also removing the field from this example to have a test coverage on this. Plus, can you also remove this line https://github.com/abhijeetkjha-google/magic-modules/blob/bugFix-361094483/mmv1/products/compute/Interconnect.yaml#L52, given the field is removed from the example. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mind also removing the field from this example to have a test coverage on this.

Since we are marking a field as optional wouldn't it be a good idea to have one test example with the field and another without the field. The compute_interconnect_basic.tf.erb contains the required field only and compute_interconnect_basic_test.tf.erb includes the optional field as well.

can you also remove this line https://github.com/abhijeetkjha-google/magic-modules/blob/bugFix-361094483/mmv1/products/compute/Interconnect.yaml#L52, given the field is removed from the example.

Sure, Done.

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind also removing the field from this example to have a test coverage on this.

Since we are marking a field as optional wouldn't it be a good idea to have one test example with the field and another without the field. The compute_interconnect_basic.tf.erb contains the required field only and compute_interconnect_basic_test.tf.erb includes the optional field as well.

Oh, right, it makes sense to have the field included in at least one test. But it looks like compute_interconnect_basic.tf.erb is only used to generate the doc instead of tests, as indicated by the flag skip_test for the example. Are you able to test it locally and make sure the field is not required for resource creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes yes, I have verified that customer_name is an optional field and is not required for resource creation.

resource "google_compute_interconnect" "<%= ctx[:primary_resource_id] %>" {
name = "<%= ctx[:vars]['interconnect_name'] %>"
customer_name = "<%= ctx[:vars]['customer_name'] %>"
interconnect_type = "DEDICATED"
link_type = "LINK_TYPE_ETHERNET_10G_LR"
location = "https://www.googleapis.com/compute/v1/projects/${data.google_project.project.name}/global/interconnectLocations/iad-zone1-1"
Expand Down
Loading