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 oidcToken field to pubsub subscription #2120

Closed
wants to merge 2 commits into from

Conversation

tmshn
Copy link
Contributor

@tmshn tmshn commented Aug 1, 2019

Added missing oidcToken field to pubsub subscription (which is equivalent of --push-auth-service-account flag of gcloud command)

Release Note for Downstream PRs (will be copied)


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

@tmshn
Copy link
Contributor Author

tmshn commented Aug 1, 2019

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 1, 2019
@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. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

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

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of 909b964.

Pull request statuses

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#1018
depends: GoogleCloudPlatform/terraform-google-conversion#151
depends: hashicorp/terraform-provider-google#4215
depends: modular-magician/ansible#344
depends: modular-magician/inspec-gcp#178

Copy link

@tysen tysen left a comment

Choose a reason for hiding this comment

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

Addition looks good but will you add either add usage as an example (in templates/terraform/examples/) (which will generate a test) or add a handwritten test to third_party/terraform/tests/resource_pubsub_subscription_test.go?

@tmshn
Copy link
Contributor Author

tmshn commented Aug 14, 2019

@tysen To test this, we need actual push endpoint with GCP IAM-auth compatible. CloudRun seems to be the best for the situation (and actually is what drove me to add this field), so I added the example (just following the below tutorial). However, Terraform lacks some features around CloudRun to complete tests (see comments in the example). Hence, I just set skip_test: true
https://cloud.google.com/run/docs/tutorials/pubsub

@tmshn
Copy link
Contributor Author

tmshn commented Aug 14, 2019

Or maybe, better to create another pr to add url field to the CloudRun service resource first 🤔


push_config {
# TODO: this must be CloudRun's url, which is not currently exposed on the Terraform
# push_endpoint = "${google_cloud_run_service.<%= ctx[:primary_resource_id] %>.status.url}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this pull requests will fix this: #2185

@tysen tysen requested review from danawillow and tysen and removed request for tysen August 16, 2019 18:40
@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, ff275d1.

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.
Ansible already has an open PR.
InSpec already has an open PR.

New Pull Requests

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

@danawillow
Copy link
Contributor

I'm going to hold off on the review until #2185 is done and the tests here use the new url field, so that we can run them automatically.

@rileykarson
Copy link
Member

FWIW, I'm not sure #2185 will solve this- it makes the url field available, but the field isn't initially populated (in our tests so far, at least). This interpolation won't work as expected.

@tmshn
Copy link
Contributor Author

tmshn commented Aug 29, 2019

@rileykarson is pointing out a very critical thing; it seems we need hashicorp/terraform-provider-google#4335 for this test to work

@danawillow
Copy link
Contributor

Back from vacation- looks like that issue was a dupe of hashicorp/terraform-provider-google#4091, which was assigned to me. I realistically won't be able to even start working on it for at least another 2-3 weeks, so I unassigned it in case anyone else wants to look at it.

@rileykarson
Copy link
Member

Adding for completeness- @tmshn mentioned in hashicorp/terraform-provider-google#4091 they were interested in working on the issue. Once that's done, we can proceed with this PR as we'll be able to test it.

@tmshn
Copy link
Contributor Author

tmshn commented Oct 24, 2019

@rileykarson @danawillow @slevenick Maybe this PR is no more needed due to merge of #2440 ?

@danawillow
Copy link
Contributor

Yup, that looks to be the case. Sorry for taking so long on the review, and thank you for your patience :)

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.

6 participants