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.

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

Don't send update request if updatemask is empty for resources with terraform_labels field #9154

Merged
merged 4 commits into from
Oct 4, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
29 changes: 25 additions & 4 deletions mmv1/products/networksecurity/AddressGroup.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,28 @@ references:
"Use AddressGroups": "https://cloud.google.com/vpc/docs/use-address-groups-firewall-policies"
api: "https://cloud.google.com/traffic-director/docs/reference/network-security/rest/v1beta1/organizations.locations.addressGroups"
# Prevent the operation from being generated so we can call the custom one.
autogen_async: false
autogen_async: true
rileykarson marked this conversation as resolved.
Show resolved Hide resolved
async: !ruby/object:Api::OpAsync
operation: !ruby/object:Api::OpAsync::Operation
path: 'name'
base_url: '{{op_id}}'
wait_ms: 1000
timeouts: !ruby/object:Api::Timeouts
insert_minutes: 20
update_minutes: 20
delete_minutes: 20
result: !ruby/object:Api::OpAsync::Result
path: 'response'
status: !ruby/object:Api::OpAsync::Status
path: 'done'
complete: true
allowed:
- true
- false
error: !ruby/object:Api::OpAsync::Error
path: 'error'
message: 'message'
include_project: true
import_format: ["{{%parent}}/locations/{{location}}/addressGroups/{{name}}"]
examples:
- !ruby/object:Provider::Terraform::Examples
Expand All @@ -53,9 +74,9 @@ examples:
project: :PROJECT_NAME
custom_code:
!ruby/object:Provider::Terraform::CustomCode # Calling custom operation that are different from other network security resources.
post_create: templates/terraform/post_create/network_security_address_group_operation.go.erb
post_update: templates/terraform/post_update/network_security_address_group_operation.go.erb
post_delete: templates/terraform/post_delete/network_security_address_group_operation.go.erb
pre_create: templates/terraform/pre_create/network_security_address_group_operation.go.erb
pre_update: templates/terraform/pre_update/network_security_address_group_operation.go.erb
pre_delete: templates/terraform/pre_delete/network_security_address_group_operation.go.erb
parameters:
- !ruby/object:Api::Type::String
name: parent
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ if d.HasChange("connected_repositories") {
for _, repo := range removeRepos {
obj := make(map[string]interface{})
obj["connectedRepository"] = repo
res, err = transport_tpg.SendRequest(transport_tpg.SendRequestOptions{
_, err = transport_tpg.SendRequest(transport_tpg.SendRequestOptions{
Config: config,
Method: "POST",
Project: billingProject,
Expand Down Expand Up @@ -61,7 +61,7 @@ if d.HasChange("connected_repositories") {
return err
}

res, err = transport_tpg.SendRequest(transport_tpg.SendRequestOptions{
res, err := transport_tpg.SendRequest(transport_tpg.SendRequestOptions{
Config: config,
Method: "POST",
Project: billingProject,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
project = billingProject
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
project = billingProject
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
project = billingProject
Copy link
Member Author

@zli82016 zli82016 Oct 4, 2023

Choose a reason for hiding this comment

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

Actually, this line is before getting billingProject in update and delete method in the generated file and isn't working as expected
https://github.com/modular-magician/terraform-provider-google-beta/blob/be49b211522d9e2b34b84ff66268f8a9f8be5bf0/google-beta/services/networksecurity/resource_network_security_address_group.go#L366

Is it safe to move the pre_update just from line 776 to line 794 before the line of sendRequest in template file resource.erb? (The same action for pre_delete)

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Let's make these "" in that case. Can you file an issue to fix the inconsistency in where pre_ methods are? That should happen in a dedicated PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I will file an issue.

4 changes: 2 additions & 2 deletions mmv1/templates/terraform/resource.erb
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{
billingProject = bp
}

<% if object.update_mask && field_specific_update_methods(object.root_properties) -%>
<% if object.update_mask -%>
// if updateMask is empty we are not updating anything so skip the post
if len(updateMask) > 0 {
<% end -%>
Expand Down Expand Up @@ -838,7 +838,7 @@ if len(updateMask) > 0 {
}
<% end -%>
<% end -%>
<% if object.update_mask && field_specific_update_methods(object.root_properties) -%>
<% if object.update_mask -%>
}
<% end -%>
<% end # if !object.immutable -%>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,43 @@ func TestAccPubsubTopic_schema(t *testing.T) {
})
}

func TestAccPubsubTopic_migration(t *testing.T) {
acctest.SkipIfVcr(t)
t.Parallel()

topic := fmt.Sprintf("tf-test-topic-%s", acctest.RandString(t, 10))

oldVersion := map[string]resource.ExternalProvider{
"google": {
VersionConstraint: "4.84.0", // a version that doesn't separate user defined labels and system labels
Source: "registry.terraform.io/hashicorp/google",
},
}

acctest.VcrTest(t, resource.TestCase{
PreCheck: func() { acctest.AccTestPreCheck(t) },
CheckDestroy: testAccCheckPubsubTopicDestroyProducer(t),
Steps: []resource.TestStep{
{
Config: testAccPubsubTopic_update(topic, "foo", "bar"),
ExternalProviders: oldVersion,
},
{
Config: testAccPubsubTopic_update(topic, "foo", "bar"),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
},
{
ResourceName: "google_pubsub_topic.foo",
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
ImportStateId: topic,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"labels", "terraform_labels"},
},
},
})
}

func testAccPubsubTopic_update(topic, key, value string) string {
return fmt.Sprintf(`
resource "google_pubsub_topic" "foo" {
Expand Down