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

[Terraform] autogenerate Pubsub Topics/Subscriptions (+labels) #1088

Merged

Conversation

drebes
Copy link
Member

@drebes drebes commented Dec 19, 2018


[all]

Adds labels to Pubsub Topics & Subscriptions.

[terraform]

Autogenerate Pubsub Topics & Subscriptions, including labels support. Fixes hashicorp/terraform-provider-google#1500

[terraform-beta]

[ansible]

[inspec]

@drebes drebes force-pushed the terraform-pubsub-labels branch 2 times, most recently from 6c8958e to e8f6e70 Compare December 22, 2018 21:37
@drebes drebes changed the title [Terraform] autogenerate Pubsub Topic (+labels) [Terraform] autogenerate Pubsub Topics/Subscriptions (+labels) Dec 22, 2018
@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.

@rambleraptor
Copy link
Contributor

Hello! Thanks so much for submitting this PR. Sorry we didn't notice it.

@rileykarson is going to help you out. If you could rebase the PR, we'll run it through our CI system and get this review going + merged as quickly as possible.

@drebes
Copy link
Member Author

drebes commented Feb 12, 2019

Rebased.

@rileykarson
Copy link
Member

Thanks! Running the Magician now. At first glance the change seems good, I'll be able to do a full review with downstreams generated.

@rileykarson
Copy link
Member

rileykarson commented Feb 13, 2019

Seeing this error, I think that the variable was removed recently so you can likely blow it away:

RuntimeError: Extraneous variable 'exports' in TopicApi::Resource

edit: yep, see https://github.com/GoogleCloudPlatform/magic-modules/pull/1345/files#diff-293147022553799829844cb248360ba3L30

@drebes drebes force-pushed the terraform-pubsub-labels branch from 8e6ac1a to 7fa64fd Compare February 13, 2019 00:27
@drebes
Copy link
Member Author

drebes commented Feb 13, 2019

Sorry, should be good now.

@rileykarson
Copy link
Member

Ah, Ansible requires fields to be separately versioned. You'll need to add versions for the field similar to this:

version_added: '2.7'

ERROR: Found 2 validate-modules issue(s) which need to be resolved:
ERROR: lib/ansible/modules/cloud/google/gcp_pubsub_subscription.py:0:0: E309 version_added for new option (labels) should be 2.8. Currently 0.0
ERROR: lib/ansible/modules/cloud/google/gcp_pubsub_topic.py:0:0: E309 version_added for new option (labels) should be 2.8. Currently 0.0

@drebes drebes force-pushed the terraform-pubsub-labels branch from 7fa64fd to 8bebf80 Compare February 13, 2019 21:38
@drebes
Copy link
Member Author

drebes commented Feb 13, 2019

Sorry, missed that. Should be good now.

@drebes drebes force-pushed the terraform-pubsub-labels branch from 8bebf80 to 30d8bce Compare February 13, 2019 21:46
@modular-magician
Copy link
Collaborator

I am a robot that works on MagicModules PRs!

I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#432
depends: hashicorp/terraform-provider-google#3043
depends: modular-magician/ansible#191
depends: modular-magician/inspec-gcp#109

@rileykarson
Copy link
Member

rileykarson commented Feb 13, 2019

There we go! Reviewing Terraform now.

@rambleraptor @slevenick can you review your downstreams?

Edit: ah @rambleraptor beat me to it inside the downstream PR

@drebes
Copy link
Member Author

drebes commented Feb 17, 2019

Actually, the PATCH from gcloud works, and contains the modified fields inside the subscription object:

==== request start ====
uri: https://pubsub.googleapis.com/v1/projects/drebes-playground-alpha-5ae4/subscriptions/a-sub?alt=json
method: PATCH
== headers start ==
Authorization: --- Token Redacted ---
accept: application/json
accept-encoding: gzip, deflate
content-length: 149
content-type: application/json
user-agent: google-cloud-sdk x_Tw5K8nnjoRAqULM9PFAC2b gcloud/234.0.0 command/gcloud.pubsub.subscriptions.update invocation-id/1547efab192a4ca4a09cba70a36e36d0 environment/None environment-version/None interactive/True from-script/False python/2.7.15 (Macintosh; Intel Mac OS X 18.2.0)
== headers end ==
== body start ==
{"subscription": {"ackDeadlineSeconds": 30, "name": "projects/drebes-playground-alpha-5ae4/subscriptions/a-sub"}, "updateMask": "ackDeadlineSeconds"}
== body end ==
==== request end ====
---- response start ----
-- headers start --
-content-encoding: gzip
alt-svc: quic=":443"; ma=2592000; v="44,43,39"
cache-control: private
content-length: 339
content-type: application/json; charset=UTF-8
date: Sun, 17 Feb 2019 19:19:48 GMT
server: ESF
status: 200
transfer-encoding: chunked
vary: Origin, X-Origin, Referer
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
-- headers end --
-- body start --
{
  "name": "projects/drebes-playground-alpha-5ae4/subscriptions/a-sub",
  "topic": "projects/drebes-playground-alpha-5ae4/topics/a-topic",
  "pushConfig": {
    "attributes": {
      "x-goog-version": "v1"
    }
  },
  "ackDeadlineSeconds": 30,
  "messageRetentionDuration": "604800s",
  "expirationPolicy": {
    "ttl": "2678400s"
  }
}

-- body end --
total round trip time (request+response): 1.489 secs
---- response end ----

How can I force it to update the modified fields inside the subscription object (and updateMask as field as well, not parameter)?

@drebes drebes force-pushed the terraform-pubsub-labels branch from 315f9e2 to d99ef9b Compare February 17, 2019 20:03
@drebes
Copy link
Member Author

drebes commented Feb 17, 2019

I added an update_encoder and now it all looks fine. 👍

@rileykarson
Copy link
Member

Yep, an update encoder is the right tool for a case like that 👍

Generating downstreams and making another pass once they're done.

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

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#450
depends: hashicorp/terraform-provider-google#3080
depends: modular-magician/ansible#193
depends: modular-magician/inspec-gcp#111

@rileykarson
Copy link
Member

Looks like a Magician update changed the branch we PR off of, these new downstreams will supersede the old ones.

@rileykarson
Copy link
Member

Since the original downstreams are still getting updated, I commented on hashicorp/terraform-provider-google-beta#432 again.

While adding resource/product specific reference (self_link/name) validation is a good idea eventually, I don't think that handwriting it is the right balance of maintenance vs benefit. We can continue to reuse generic functions for handling short names / partial URIs / reference links & keep consistent with most existing resources; that'll make an eventual migration easier.

My apologies again for the fiddlyness of name-style references in MM, especially when converting handwritten resources. I'm passively thinking about how to improve the situation (https://github.com/GoogleCloudPlatform/magic-modules/issues/937), the problem is that many APIs have adopted slightly different patterns vs the relative consistency of self links in the past.

@drebes drebes force-pushed the terraform-pubsub-labels branch 3 times, most recently from d293c97 to c264f4e Compare February 19, 2019 22:12
@drebes
Copy link
Member Author

drebes commented Feb 19, 2019

I've pushed my changes. If you can regenerate the downstream PRs and give it a new look...

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

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-provider-google already has an open PR.
Ansible already has an open PR.
Ansible already has an open PR.

New Pull Requests

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

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Thanks for the great work @drebes!

Fiddling with migrations of name-style resources to MM can be a little trying, so thanks again for your patience in working out how we continue to support the existing behaviour while working within the confines of MM.

drebes and others added 2 commits February 19, 2019 23:14
Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.
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.

5 participants