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 endpoints service IAM #2318

Merged
merged 10 commits into from
Feb 13, 2020

Conversation

slevenick
Copy link
Contributor

@slevenick slevenick commented Sep 14, 2019

Fixes hashicorp/terraform-provider-google#3617

Release Note for Downstream PRs (will be copied)

google_endpoints_service_iam_binding
google_endpoints_service_iam_policy
google_endpoints_service_iam_member

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 489f02d.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1139
depends: GoogleCloudPlatform/terraform-google-conversion#201
depends: hashicorp/terraform-provider-google#4456

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, d7b4ac1.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

# See the License for the specific language governing permissions and
# limitations under the License.

# TODO(nelsonjr): Make all Zone and Region resource ref
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this can be removed

versions:
- !ruby/object:Api::Product::Version
name: ga
base_url: https://servicemanagement.googleapis.com/v1/
Copy link
Contributor

Choose a reason for hiding this comment

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

So one thing that appears to have been generated is the endpoints custom endpoint, which is a bit tricky since the existing endpoints_service resource uses the service management custom endpoint. I think it would be better if there were just one custom endpoint for the endpoints_service resource and its associated IAM resources.

Copy link
Contributor Author

@slevenick slevenick Sep 18, 2019

Choose a reason for hiding this comment

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

So change this to the service management product, and override the name via legacy name? It may be hard to match up the IAM resource to be named similarly to the google_endpoints_service resource

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely going to need some thought on how to do it well. One option would be to create a function to determine the custom endpoint name, which would return a custom_endpoint_name value set in terraform.yaml, and default to whatever we currently use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the product to ServiceManagement and then overrode the name of the IAM resources to match the existing handwritten resource

@slevenick slevenick force-pushed the endpoints-service-iam branch from d7b4ac1 to a5870b0 Compare February 10, 2020 20:22
@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 ( 5 files changed, 558 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 558 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 3 insertions(+))

1 similar comment
@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 ( 5 files changed, 558 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 558 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 3 insertions(+))

@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 ( 6 files changed, 559 insertions(+), 16 deletions(-))
Terraform Beta: Diff ( 6 files changed, 559 insertions(+), 16 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 14 deletions(-))

@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 ( 6 files changed, 559 insertions(+), 18 deletions(-))
Terraform Beta: Diff ( 6 files changed, 559 insertions(+), 18 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 14 deletions(-))

@slevenick slevenick requested a review from danawillow February 10, 2020 23:02
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Can you also update google.erb for the sidebar?

@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 ( 7 files changed, 568 insertions(+), 18 deletions(-))
Terraform Beta: Diff ( 7 files changed, 568 insertions(+), 18 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 14 deletions(-))

@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 ( 7 files changed, 568 insertions(+), 18 deletions(-))
Terraform Beta: Diff ( 7 files changed, 568 insertions(+), 18 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 14 deletions(-))

@slevenick slevenick requested a review from danawillow February 11, 2020 00:19
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

You'll also want to update the changelog note to whatever we do differently now than we did in september

@@ -173,17 +173,6 @@ var RuntimeConfigCustomEndpointEntry = &schema.Schema{
}, RuntimeConfigDefaultBasePath),
}

var ServiceManagementDefaultBasePath = "https://servicemanagement.googleapis.com/v1/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think we do need to leave these in until 4.0.0 so that anyone that was setting the custom endpoint still can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh gosh, I keep changing the product name to make the docs line up right but the product name also goes in here 🤦‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by changing the docs to use the product's display name and moving the product that produces this IAM resource to use the same namespace as the existing handwritten one.

@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 ( 10 files changed, 580 insertions(+), 30 deletions(-))
Terraform Beta: Diff ( 10 files changed, 580 insertions(+), 30 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 14 deletions(-))

@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 ( 23 files changed, 614 insertions(+), 98 deletions(-))
Terraform Beta: Diff ( 24 files changed, 617 insertions(+), 103 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 14 deletions(-))

@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 ( 23 files changed, 632 insertions(+), 82 deletions(-))
Terraform Beta: Diff ( 24 files changed, 636 insertions(+), 86 deletions(-))
TF Conversion: Diff ( 2 files changed, 4 insertions(+), 14 deletions(-))

@slevenick slevenick requested a review from danawillow February 12, 2020 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for a google_endpoints_service_iam_policy/member/binding resource
5 participants