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.

Sign up for GitHub

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

Jump to bottom

Signed CDN urls for backend services #1604

Merged
merged 4 commits into from
Apr 3, 2019
Merged

Signed CDN urls for backend services #1604

merged 4 commits into from
Apr 3, 2019

Conversation

emilymye
Copy link
Contributor

@emilymye emilymye commented Apr 1, 2019

Summary:

  • Generate backend services signed url key (tests, docs, etc)
  • Add exception to autogenerated example tests template so people don't use autogenerated tests for non-importable resources
  • Add signedUrlMaxCacheAgeSecs to BackendService (I expect it will get overriden by Generate BackendService in Terraform #1595, or I will remove those files from this PR if they get merged first)

I decided to make this a separate resource from backend_bucket_signed_url_key because I didn't like the alternatives I thought of (enum of "backendBuckets" or "backendServices" to sub into URLs), and because reading them from the backend object might diverge eventually since backend_buckets structure is much more simple than services.

[all]

[terraform]

Signed CDN urls for backend services

[terraform-beta]

Signed CDN urls for backend services

[ansible]

[inspec]

@emilymye emilymye requested review from rileykarson and chrisst April 2, 2019 00:19
@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 878d25f9e539e6e64c985a64514e62b544b6d541.

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#577
depends: GoogleCloudPlatform/terraform-google-conversion#47
depends: hashicorp/terraform-provider-google#3359

@emilymye emilymye force-pushed the signed_urls_backend_services branch from dc40b2b to 96a0094 Compare April 2, 2019 18:36
rileykarson
rileykarson reviewed Apr 2, 2019
View reviewed changes
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.

Ooops I missed the notification on this, was just trawling PRs and noticed I'd been assigned. Do you mind updating post-#1595?

@emilymye emilymye force-pushed the signed_urls_backend_services branch from 96a0094 to cba9b50 Compare April 2, 2019 22:59
@emilymye
Copy link
Contributor Author

emilymye commented Apr 2, 2019

@rileykarson 👍

@emilymye emilymye force-pushed the signed_urls_backend_services branch from cba9b50 to 818d2cc Compare April 2, 2019 23:06
@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, 818d2cc.

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.

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

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
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.

chrisst
chrisst approved these changes Apr 3, 2019
View reviewed changes
Copy link
Contributor

@chrisst chrisst left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Only minor stuff, otherwise 👍

templates/terraform/examples/base_configs/test_file.go.erb
@@ -10,6 +10,8 @@ import (
"github.com/hashicorp/terraform/helper/resource"
)
<%
raise 'skip_test should be true if resource is not importable' \
Copy link
Contributor

Choose a reason for hiding this comment

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

What about skipping the import step when generating tests instead? I'm not a fan of 'knowing' which configurations need to be used together and finding out at build time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Import is pretty important for the autogenerated tests since we use it to confirm the attributes. In general, I don't really anticipate exclude_import being used much (i.e. pretty much when the resource contains sensitive information that cannot be returned) and the verification of test resources behavior will probably be unique, so it may not make sense?

products/compute/terraform.yaml Outdated
@@ -234,6 +235,33 @@ overrides: !ruby/object:Overrides::ResourceOverrides
diff_suppress_func: 'compareSelfLinkOrResourceName'
timeoutSec: !ruby/object:Overrides::Terraform::PropertyOverride
default_from_api: true
BackendServiceSignedUrlKey: !ruby/object:Overrides::Terraform::ResourceOverride
exclude_import: true
is_fine_grained_resource: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a property/convenience method based on the existence of nested_query rather than its own property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:exclude_import is less because it's fine-grained, more because this has two fields (name and key) and one is sensitive, so it doesn't really make sense to allow for import. Otherwise, we'd have to supply an empty key value, and the user would be asked to apply the key value, which would just recreate the key.

Also, I want to use nested_query for collections (i.e. instead of having self_link_query for list-only resources) which I do think the current validator code doesn't handle correctly but should include still, vs fine-grained which make less sense to include since they can't be tied to a specific CAI resource.

@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, 253a40c.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
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.

emilymye and others added 4 commits April 3, 2019 18:31
@modular-magician modular-magician force-pushed the signed_urls_backend_services branch from 253a40c to 309895e Compare April 3, 2019 18:32
@modular-magician modular-magician merged commit f0773a2 into GoogleCloudPlatform:master Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rileykarson rileykarson rileykarson left review comments

@chrisst chrisst chrisst approved these changes

Assignees
No one assigned
Labels
automerged cla: yes downstream-generated terraform-google-conversion terraform-provider-google terraform-provider-google-beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@emilymye @modular-magician @chrisst @rileykarson @googlebot