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 support for cloudrun ports #3748

Merged
merged 9 commits into from
Jul 28, 2020
Merged

Conversation

dj80hd
Copy link
Contributor

@dj80hd dj80hd commented Jul 15, 2020

Fixes: hashicorp/terraform-provider-google#5539

API Reference:
https://cloud.google.com/run/docs/reference/rest/v1/RevisionSpec#ContainerPort
https://cloud.google.com/run/docs/reference/rest/v1/RevisionSpec#Container

Release Note Template for Downstream PRs (will be copied)

cloudrun: added `ports` field to `google_cloud_run_service` `templates.spec.containers`

@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.

@slevenick, 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 ( 3 files changed, 146 insertions(+))
Terraform Beta: Diff ( 3 files changed, 146 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 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 ( 3 files changed, 146 insertions(+))
Terraform Beta: Diff ( 3 files changed, 146 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 insertions(+))

@dj80hd
Copy link
Contributor Author

dj80hd commented Jul 16, 2020

After more thinking and testing, I realized this solution may not be backwards compatible.

@slevenick
Copy link
Contributor

After more thinking and testing, I realized this solution may not be backwards compatible.

Why is that? Adding a new field shouldn't be a problem

@dj80hd
Copy link
Contributor Author

dj80hd commented Jul 16, 2020

Why is that? Adding a new field shouldn't be a problem

Sorry, false alarm. All tests pass as expected now.

@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, 148 insertions(+))
Terraform Beta: Diff ( 3 files changed, 148 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 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 ( 3 files changed, 150 insertions(+))
Terraform Beta: Diff ( 3 files changed, 150 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 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 ( 3 files changed, 150 insertions(+))
Terraform Beta: Diff ( 3 files changed, 150 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 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 ( 3 files changed, 150 insertions(+))
Terraform Beta: Diff ( 3 files changed, 150 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 insertions(+))

@dj80hd dj80hd requested a review from slevenick July 21, 2020 15:24
Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

It looks like this change is causing tests to fail because the ports field is filled out by the API if it is not specified. Adding default_from_api: true for this field in the products/cloudrun/terraform.yaml will mark this field as Computed in the terraform schema, which should allow the field to be set by the API without issue

@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, 151 insertions(+))
Terraform Beta: Diff ( 3 files changed, 151 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 insertions(+))

@dj80hd
Copy link
Contributor Author

dj80hd commented Jul 23, 2020

It looks like this change is causing tests to fail

Can i run these tests locally? I made the suggested change but am not sure if i am testing correctly.

My current test method is:

  1. Generate provider code with bundle exec compiler -p "products/cloudrun" -v "ga" -e terraform -o "$GOPATH/src/github.com/terraform-providers/terraform-provider-google"

  2. Build the provider with make build test

  3. Run acceptance test in provider directory: make testacc TEST=./google TESTARGS='-run TestAccCloudRunService_cloudRunServiceUpdate'

  4. Do some real terraform sanity tests using locally built provider

I did notice some lint errors unrelated to my change.

I also had to manually revert some unrelated, unexpected import changes like these.
The diff report from modular-magician looked fine, however, so i just ignored this.

@dj80hd dj80hd requested a review from slevenick July 23, 2020 18:07
@slevenick
Copy link
Contributor

It looks like this change is causing tests to fail

Can i run these tests locally? I made the suggested change but am not sure if i am testing correctly.

My current test method is:

  1. Generate provider code with bundle exec compiler -p "products/cloudrun" -v "ga" -e terraform -o "$GOPATH/src/github.com/terraform-providers/terraform-provider-google"
  2. Build the provider with make build test
  3. Run acceptance test in provider directory: make testacc TEST=./google TESTARGS='-run TestAccCloudRunService_cloudRunServiceUpdate'
  4. Do some real terraform sanity tests using locally built provider

I did notice some lint errors unrelated to my change.

I also had to manually revert some unrelated, unexpected import changes like these.
The diff report from modular-magician looked fine, however, so i just ignored this.

Yes, generally that is the way to run the generated tests. I'd suggest running a more broad suite of tests occasionally to make sure that your changes don't impact other uses of the cloud run resource.

make testacc TEST=./google TESTARGS='-run TestAccCloudRunService' is what I'm running to check this

@dj80hd
Copy link
Contributor Author

dj80hd commented Jul 23, 2020

Thanks.
make testacc TEST=./google TESTARGS='-run TestAccCloudRunService' now passes all tests.

Copy link
Contributor

@slevenick slevenick 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, just one docs change and then we are good to go!

products/cloudrun/api.yaml Outdated Show resolved Hide resolved
@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, 151 insertions(+))
Terraform Beta: Diff ( 3 files changed, 151 insertions(+))
TF Conversion: Diff ( 1 file changed, 55 insertions(+))

@dj80hd dj80hd requested a review from slevenick July 28, 2020 18:42
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.

Allow specifying 'container_port' and 'request_timeout' for google_cloud_run_service
4 participants