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 "inbound_services" to google_app_engine_standard_app_version #3537

Conversation

pmarschik
Copy link
Contributor

Release Note Template for Downstream PRs (will be copied)

appengine: add `inbound_services` to `StandardAppVersion` resource

References

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@danawillow, please review this PR or find an appropriate assignee.

@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 ( 2 files changed, 35 insertions(+))
Terraform Beta: Diff ( 2 files changed, 35 insertions(+))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
Inspec: Diff ( 4 files changed, 7 insertions(+))

@danawillow
Copy link
Contributor

Holding off on the review until the CLA is signed.

@pmarschik
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

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 add a test (or modify an existing one) that sets this field? Since it's updatable, TestAccAppEngineStandardAppVersion_update is probably a good place for it.

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@c2thorn, please review this PR or find an appropriate assignee.

@modular-magician modular-magician requested a review from c2thorn May 22, 2020 12:06
@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 ( 3 files changed, 39 insertions(+))
Terraform Beta: Diff ( 3 files changed, 39 insertions(+))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
Inspec: Diff ( 4 files changed, 7 insertions(+))

@pmarschik pmarschik requested a review from danawillow May 26, 2020 12:58
@danawillow danawillow removed the request for review from c2thorn May 26, 2020 16:29
@@ -454,6 +454,11 @@ objects:
required: true
description: |
The format should be a shell command that can be fed to bash -c.
- !ruby/object:Api::Type::Array
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend making this a set instead (is_set: true in terraform.yaml) because the order these are specified in shouldn't matter.

@@ -84,6 +84,8 @@ resource "google_app_engine_standard_app_version" "foo" {
}
}

inbound_services = ["INBOUND_SERVICE_WARMUP"]
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 test having multiple values in this? (not in a separate test, just put like 3ish values in this array)

@pmarschik pmarschik force-pushed the app_engine_standard_app_version_inbound_services branch from 7f27cc3 to 67aa69b Compare May 29, 2020 09:24
@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@SirGitsalot, please review this PR or find an appropriate assignee.

@pmarschik pmarschik requested a review from danawillow May 29, 2020 09:28
@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 ( 4 files changed, 51 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 4 files changed, 51 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 12 insertions(+))
Inspec: Diff ( 4 files changed, 7 insertions(+))

@danawillow danawillow removed the request for review from SirGitsalot May 29, 2020 17:21
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.

Looks good, thanks @pmarschik!

Converting from a list --> set on an existing resource has the potential to be a breaking change, since people could have been indexing into an array during interpolation. However, I think in this case that since it's a simple list of strings, it should be pretty safe so I'm fine with the change.

@danawillow danawillow merged commit 7803d55 into GoogleCloudPlatform:master Jun 2, 2020
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.

4 participants