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

add fine-grained resource for service perimeter resource #3052

Merged

Conversation

danawillow
Copy link
Contributor

Fixes hashicorp/terraform-provider-google#4509.

In order to do this I had to make some changes to nested_query to allow it to add/remove an element of an array which is part of a nested object. I got it to work in this case by wrapping the object down the chain, so the patch request in this case would look like:

{
  "status": {
    "resources": [...]
  }
}

Notably, this doesn't merge the resources block into the status object that was returned from the API, but instead just wraps it. This works fine in this case because this API uses an update mask, so it can modify resources without changing anything else within status, but isn't future-proof if a similarly nested resource comes along that doesn't use an update mask.

Release Note Template for Downstream PRs (will be copied)

`google_access_context_manager_service_perimeter_resource`

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff

Copy link
Contributor

@emilymye emilymye left a comment

Choose a reason for hiding this comment

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

LGTM - some nits/small validation things

third_party/terraform/utils/bootstrap_utils_test.go Outdated Show resolved Hide resolved
"<%= list_key %>": updatedItems,
}, nil
}
<% object.nested_query.keys.reverse[1..-1].each do |k| -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we:

  • check for array length
  • leave a comment here or in nested_query saying we're reconstructing the nested patch request using the keys? I know this is partially my very undocumented code, but it's a little clearer why we are doing fun indexing that way

Also, fun ruby thing - reverse_each does the same thing as .reverse.each without copying the array (in .reverse).

Copy link
Contributor

@emilymye emilymye Feb 1, 2020

Choose a reason for hiding this comment

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

we can also do a reduce here if you're interested in changing this output - I think the go would look nice but the ruby ERB less nice either way! It'd be like:

wrappedObj := <%= 
  object.nested_query.keys
            .reverse[1..-1]
            .reduce("res") { |res, k| "map[string]interface{}{\n\t#{k}:#{res},\n}" }
-%>

to print out:

wrappedObj := map[string]interface{
   "k0": map[string]interface{
      "k1": ...
         "k_last": res,
      },
   },
}

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, I opted for a bit of a compromise between the two. The one with reduce looks nice for this resource but it would need some extra handling for resources with only one key, which is the majority of them.
I also didn't bother checking the array length because it seems to succeed just fine if the array is too small (the slice of the array will just be empty).

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 21 insertions(+), 774 deletions(-))
Terraform Beta: Diff ( 10 files changed, 21 insertions(+), 777 deletions(-))

@danawillow
Copy link
Contributor Author

Also the magician is wrong, my new resource does not delete 700+ lines of code. Fix for that is out for review now.

@danawillow danawillow merged commit 3f0be87 into GoogleCloudPlatform:master Feb 3, 2020
@danawillow danawillow deleted the tf-4509-vpc-perim-project branch February 3, 2020 22:29
nathkn pushed a commit to nathkn/magic-modules that referenced this pull request May 18, 2020
…Platform#3052)

* add fine-grained resource for service perimeter resource

* exclude from inspec

* add sidebar entry and change acctest import

* comments, readability
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add granular VPC Service-Control resource
4 participants