-
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
Dataflow flex-templates resource #3520
Conversation
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. @rileykarson, please review this PR or find an appropriate assignee. |
a8e3101
to
de0fd40
Compare
Upon further thinking, I believe it makes most sense to combine |
5014a12
to
a99aacb
Compare
1 similar comment
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.
Do you mind showing me how Flex Templates end up using the same endpoint? It seems they've got a different gcloud resource (which are normally strongly linked to endpoints) as well as this CURL example which appears to hit a flex-specific endpoint: https://dataflow.googleapis.com/v1b3/projects/$PROJECT/locations/$LOCATION/flexTemplates
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. @emilymye, please review this PR or find an appropriate assignee. |
@rileykarson You're right, I was having a brain fart. I've retracted my proposal to combine these resources and will finish off |
@rileykarson @emilymye I'd love to get some feedback on the PR now. I can see that I have a failure in the cloudbuild trigger, but I'm unable to see what it is. |
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.
Overall looking strong- I left some nitpicks and comments about where we'll need version guards. I think we need to pass generation for the downstream to update and for me to run the tests.
third_party/terraform/resources/resource_dataflow_flex_template_job.go.erb
Show resolved
Hide resolved
third_party/terraform/resources/resource_dataflow_flex_template_job.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/resources/resource_dataflow_flex_template_job.go.erb
Show resolved
Hide resolved
third_party/terraform/resources/resource_dataflow_flex_template_job.go.erb
Outdated
Show resolved
Hide resolved
if err != nil { | ||
return err | ||
} | ||
d.SetId(job.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.
just curious- is the id of the job expected to change here?
third_party/terraform/resources/resource_dataflow_flex_template_job.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_dataflow_flex_template_job_test.go
Outdated
Show resolved
Hide resolved
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Oops! It looks like no changelog entry is attached to this PR. Please include a release note block in the PR body, as described in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md:
|
Thank you for adding me. Do you want me to review something specific? Adding, people who directly work on templates: @azurezyq @arvindram03 |
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.
Just a couple schema issues right now. They're the reason for the failed trigger.
--- FAIL: TestProvider (0.04s)
provider_test.go:377: err: 1 error occurred:
* resource google_dataflow_flex_template_job: No Update defined, must set ForceNew on: []string{"on_delete", "labels"}
third_party/terraform/resources/resource_dataflow_flex_template_job.go.erb
Show resolved
Hide resolved
third_party/terraform/resources/resource_dataflow_flex_template_job.go.erb
Outdated
Show resolved
Hide resolved
ForceNew: true, | ||
}, | ||
|
||
"on_delete": { |
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 believe we want a blank update method defined. That will make Terraform persist the value for on_delete
into state. If there's no method at all, it requires this field to recreate the resource.
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.
Do you mind rebasing on top of master? Something changed between the commit this was created at and now that causes our CI to fail, probably a setting that's encoded in a file inside the repo. I've had it happen on a few of my PRs as well.
Adds first pass for flex-templates docs website wip Deleting unused
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. @danawillow, please review this PR or find an appropriate assignee. |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122436" |
third_party/terraform/tests/resource_dataflow_flex_template_job_test.go.erb
Show resolved
Hide resolved
third_party/terraform/tests/resource_dataflow_flex_template_job_test.go.erb
Show resolved
Hide resolved
third_party/terraform/tests/resource_dataflow_flex_template_job_test.go.erb
Show resolved
Hide resolved
third_party/terraform/tests/resource_dataflow_flex_template_job_test.go.erb
Show resolved
Hide resolved
return ` | ||
<<EOF | ||
{ | ||
"name": "Streaming Beam SQL", |
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.
Is this a complete flex template? I get the following error after making the changes noted above: Error: googleapi: Error 400: (165707124cd2c54b): Docker image not specified in the template file., badRequest
|
||
The Dataflow resource is considered 'existing' while it is in a nonterminal state. If it reaches a terminal state (e.g. 'FAILED', 'COMPLETE', 'CANCELLED'), it will be recreated on the next 'apply'. This is as expected for jobs which run continuously, but may surprise users who use this resource for other kinds of Dataflow jobs. | ||
|
||
A Dataflow job which is 'destroyed' may be "cancelled" or "drained". If "cancelled", the job terminates - any data written remains where it is, but no new data will be processed. If "drained", no new data will enter the pipeline, but any data currently in the pipeline will finish being processed. The default is "cancelled", but if a user sets `on_delete` to `"drain"` in the configuration, you may experience a long wait for your `terraform destroy` to complete. |
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.
You could link to : https://cloud.google.com/dataflow/docs/guides/stopping-a-pipeline for effects of cancel vs drain.
I do not know if destroyed
is TF terminology. Dataflow term for this is stopping
.
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.
See above.
|
||
[ To Come ...] | ||
## Note on "destroy" / "apply" | ||
There are many types of Dataflow jobs. Some Dataflow jobs run constantly, getting new data from (e.g.) a GCS bucket, and outputting data continuously. Some jobs process a set amount of data then terminate. All jobs can fail while running due to programming errors or other issues. In this way, Dataflow jobs are different from most other Terraform / Google resources. |
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.
Dataflow terminology for these two types of jobs are streaming
and batch
jobs respectively.
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 going to keep this message as-is for consistency with the existing resource at https://www.terraform.io/docs/providers/google/r/dataflow_job.html. If you're able to PR a fix (or just file an issue that would be a big help!
## Note on "destroy" / "apply" | ||
There are many types of Dataflow jobs. Some Dataflow jobs run constantly, getting new data from (e.g.) a GCS bucket, and outputting data continuously. Some jobs process a set amount of data then terminate. All jobs can fail while running due to programming errors or other issues. In this way, Dataflow jobs are different from most other Terraform / Google resources. | ||
|
||
The Dataflow resource is considered 'existing' while it is in a nonterminal state. If it reaches a terminal state (e.g. 'FAILED', 'COMPLETE', 'CANCELLED'), it will be recreated on the next 'apply'. This is as expected for jobs which run continuously, but may surprise users who use this resource for other kinds of Dataflow jobs. |
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: Replace for other kinds of Dataflow jobs.
with for Dataflow batch jobs.
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.
See above.
Type: schema.TypeString, | ||
ValidateFunc: validation.StringInSlice([]string{"cancel", "drain"}, false), | ||
Optional: true, | ||
Default: "drain", |
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.
Doc below says the default is cancel. (The default is "cancelled",
)
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.
Changing to cancel.
d.SetId("") | ||
return nil | ||
} | ||
d.SetId(job.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.
Why is this line needed L127, read this info from this field already?
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.
Doesn't hurt, but doesn't do anything- removed.
I copied the changes from this PR into #3772, superseding this one. @geojaz do you mind responding to the googlebot comment when you have the chance there? I squashed your commits into a single one, for ease of reviewability. |
Release Note Template for Downstream PRs (will be copied)
We'd like to get the beta provider for terraform to support Dataflow Flex-Templates. This is based on the hand-written
dataflow_job
resource and appears to work for flex-template CRUD activities.I will continue to iterate on this next week with some tests and to finish off the docs. I'll ping whn it's good to be reviewed.
FYI @pupamanyu @WillBeebe @cxhercules
[drive-by edit by danawillow:] Fixes hashicorp/terraform-provider-google#6656