-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Cloudrun beta implementation on alpha KNative #1820
Cloudrun beta implementation on alpha KNative #1820
Conversation
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI 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. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI 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. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi drive-by reviewer. Did you mean to commit "products/test"? |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
@danawillow These resources are working, but they don't currently have a great experience in the error scenario. For example if you create a DomainMapping with bad values it doesn't do much validation on the POST and instead returns the object where the However we can't use
I think adding custom polling behavior using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm excited to see this moving forward!
description: The location of the cloud run instance. eg us-central1 | ||
url_param_only: true | ||
required: true | ||
properties: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a few comments in, but generally I think most of these descriptions are hard to parse, and many contain information not relevant to our users. It would be nice if they could be given a once-over to try to make them a bit more user-friendly.
Also I'm generally a fan of putting things alphabetically, just to make it easier to find stuff later. Up to you though, don't worry about it if it messes with your tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I get for directly generating from REST docs, which directly generate from protos, which directly generate from....
I'll take a quick pass.
products/cloudrun/terraform.yaml
Outdated
kind: !ruby/object:Overrides::Terraform::PropertyOverride | ||
exclude: true | ||
apiVersion: !ruby/object:Overrides::Terraform::PropertyOverride | ||
output: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why output instead of exclude?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh - I'm worried that this value might be different even when querying the same endpoint. I am pretty sure that you will be able to retrieve Services created at v1alpha1 via the v1beta1 endpoint when it ships. So I thought it would be valuable down the road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's easier to add than to remove, would it make sense to wait until it becomes valuable to add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sound good.
"github.com/hashicorp/terraform/helper/resource" | ||
) | ||
|
||
func TestAccCloudrunService_cloudrunServiceUpdate(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This failed when I tried to run it:
------- Stdout: -------
=== RUN TestAccCloudrunService_cloudrunServiceUpdate
=== PAUSE TestAccCloudrunService_cloudrunServiceUpdate
=== CONT TestAccCloudrunService_cloudrunServiceUpdate
--- FAIL: TestAccCloudrunService_cloudrunServiceUpdate (4.99s)
testing.go:568: Step 2 error: errors during apply:
Error: Error updating Service "tftest-cloudrun-lmgbra": googleapi: Error 409: Conflict for resource 'tftest-cloudrun-lmgbra' for version '1563901425614000'.
on /opt/teamcity-agent/temp/buildTmp/tf-test772321307/main.tf line 2:
(source code not available)
FAIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been able to reproduce this once, but not consistently. I suspect this is a race condition due to not handling polling until ready:true.
} | ||
} | ||
|
||
# The Service is ready to be used when the "Ready" condition is True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't wait in the resource itself because of the comment that you made about Unknown sometimes being a terminal state, right? Is there any way we can give feedback on that? I feel like doing it this way just pushes the problem down to our users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary from in person conversation: It sounds like DomainMapping should be the exception and my understanding so far is that UNKNOWN
should be a truly transitional state in most cases. We decided that Service needs this before it's really usable so I filed hashicorp/terraform-provider-google#4091 and I am opening a bug against the DomainMapping endpoint to see if we can figure out a better state resolution.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few test/docs comments, but LGTM once those are resolved 🙌
resource "google_cloudrun_domain_mapping" "<%= ctx[:primary_resource_id] %>" { | ||
location = "us-central1" | ||
provider = "google-beta" | ||
name = "<%= ctx[:vars]['cloudrun_domain_name'] %>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this generated an empty name: https://github.com/terraform-providers/terraform-provider-google-beta/pull/757/files#diff-2b750590a17b245bd0d6188d7ed172b3
@@ -0,0 +1,13 @@ | |||
resource "google_cloudrun_domain_mapping" "<%= ctx[:primary_resource_id] %>" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be google_cloud_run_domain_mapping now
@@ -0,0 +1,28 @@ | |||
resource "google_cloudrun_service" "<%= ctx[:primary_resource_id] %>" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -0,0 +1,28 @@ | |||
resource "google_cloudrun_service" "<%= ctx[:primary_resource_id] %>" { | |||
name = "<%= ctx[:vars]['cloudrun_service_name'] %>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
# Due to Terraform and API limitations this is best accessed through a local variable | ||
locals { | ||
cloudrun_status = { | ||
for cond in google_cloudrun_service.default.status[0].conditions : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
products/cloudrun/api.yaml
Outdated
Resource to hold the state and status of a user's domain mapping. | ||
|
||
**Note:** Cloud Run as a product is in beta, however the REST API is currently still an alpha. | ||
Please use this with caution as it may change when the resource moves to beta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: s/resource/API (since we're in the beta provider right now, which could make things confusing)
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
add cloudrun to providers block remove all output: false to simplify the yaml Add location property for building URLs Setting required fields and hacking name Removing fields that aren't core more updates to cloudrun service Create and Read are now working Flattened the spec block to get down to relevant properties hoisted name to a top level resoruce and hard coded apiVersion with encoders WIP adding cloudrun tests Cloud Run Service Updates Get generated tests up and running. Fix documentation for flattened objects Add docs and remove fields that can't be set Adding Service udpate test Add baseurl plubming Change cloudrun to beta format Add example version Removing test products changing for beta version change to beta endpoints
Also making cloudrun encoders generic Enrich Service examples
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
34842b1
to
3388baa
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI 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. |
Publishing this as a WIP PR for early feedback (see the TODO checklist for the remaining work to be done) and get code generation working. This is an implementation of the beta version of CloudRun, however they are currently running against an alpha version of the KNative apis which means that the underlying object model could change when CloudRun moves to KNative beta. See KNative release details for more information on the beta implementation.
TODO
location
is showing up in docsRequested in hashicorp/terraform-provider-google#3421
Release Note for Downstream PRs (will be copied)