Adding Missing Cloud Build Attributes #3627

tanjinP · 2020-06-11T12:54:11Z

Ref: hashicorp/terraform-provider-google#6531
Ref: #3576

Q for reviewer/maintainer (while in draft): want to ensure I am going in the right direction with what was done thus far. Should I be adding test? Overriding in terraform.yaml? Open to hearing anything and everything 🙂

Release Note Template for Downstream PRs (will be copied)

cloudbuild: added tags, source, queue_ttl, logs_bucket, substitutions, and secrets to `google_cloudbuild_trigger`

modular-magician · 2020-06-11T12:55:14Z

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.

@SirGitsalot, please review this PR or find an appropriate assignee.

tanjinP · 2020-06-11T12:57:55Z

I have not added artifacts and options attributes just yet. Will do so after some guidance if this is going in the right direction.

modular-magician · 2020-06-11T13:00:07Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 711 insertions(+))
Terraform Beta: Diff ( 2 files changed, 711 insertions(+))
Ansible: Diff ( 2 files changed, 548 insertions(+))
TF Conversion: Diff ( 1 file changed, 286 insertions(+))
Inspec: Diff ( 9 files changed, 249 insertions(+))

modular-magician · 2020-06-11T13:00:13Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=119503"

tanjinP · 2020-06-16T18:37:33Z

@danawillow any feedback for this?

I'm not sure if the auto-assigning to @SirGitsalot is valid in light of #3592

🤔

danawillow

Looks like the right direction to me! I wouldn't suggest overriding anything in terraform.yaml until you have a specific reason why you would need to- usually I find it becomes apparent when testing my changes.

For testing, I'd recommend creating an example (see cloudbuild_trigger_filename for the current one for this resource) that sets some/all of the new fields, and then adding it to terraform.yaml. That'll generate a test that sets them. After that, I'd also recommend adding the new fields to the existing (or a new) update test in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/third_party/terraform/tests/resource_cloudbuild_trigger_test.go) so we know they can be updated.

Let me know if you have further questions!

danawillow · 2020-06-17T23:08:58Z

products/cloudbuild/api.yaml

+              - !ruby/object:Api::Type::NestedObject
+                name: 'storageSource'
+                exactly_one_of:
+                  - storageSource


Unfortunately we can't do exactly_one_of on elements of lists: hashicorp/terraform-plugin-sdk#470. I'd recommend just documenting the behavior (we actually already do that elsewhere in this resource: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/products/cloudbuild/terraform.yaml#L38-L40)

Great catch - addressed here

I copied the approach for the branchName, tagName, and commitSHA from the other ones in the file (not overriding, just putting into the description).

Looks like it's still here. Check out the generated code as well to see what it ends up doing: https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-3627-old..auto-pr-3627#diff-f0c51a3d504e195b74aeb122b2747c9eR318.

Okay explicitly removed here

…build-properties

danawillow · 2020-07-07T19:20:54Z

Hey @tanjinP, just wanted to check if you're still working on this! Let me know if you have any more questions.

tanjinP · 2020-07-07T19:24:33Z

Hey @tanjinP, just wanted to check if you're still working on this! Let me know if you have any more questions.

Appreciate you checking in @danawillow - yes I was on break for a while and started up my laptop last night to address the feedback for this PR and make tests and quickly feel asleep before doing anything 😴 😅

I will have the next pass complete by tomorrow morning and will make the PR ready for review as well as re-request you. Apologies for taking so long - I know a few folks are looking to get this so will get right on it.

…build-properties

modular-magician · 2020-07-07T23:33:30Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 711 insertions(+))
Terraform Beta: Diff ( 2 files changed, 711 insertions(+))
Ansible: Diff ( 2 files changed, 548 insertions(+))
TF Conversion: Diff ( 1 file changed, 286 insertions(+))
Inspec: Diff ( 9 files changed, 249 insertions(+))

modular-magician · 2020-07-08T14:41:28Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 809 insertions(+))
Terraform Beta: Diff ( 4 files changed, 809 insertions(+))
Ansible: Diff ( 2 files changed, 548 insertions(+))
TF Conversion: Diff ( 1 file changed, 286 insertions(+))
TF OiCS: Diff ( 4 files changed, 130 insertions(+))
Inspec: Diff ( 9 files changed, 249 insertions(+))

modular-magician · 2020-07-08T15:16:31Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 812 insertions(+))
Terraform Beta: Diff ( 4 files changed, 812 insertions(+))
Ansible: Diff ( 2 files changed, 557 insertions(+))
TF Conversion: Diff ( 1 file changed, 286 insertions(+))
TF OiCS: Diff ( 4 files changed, 130 insertions(+))
Inspec: Diff ( 9 files changed, 249 insertions(+))

tanjinP · 2020-07-08T15:17:59Z

Looks like the right direction to me! I wouldn't suggest overriding anything in terraform.yaml until you have a specific reason why you would need to- usually I find it becomes apparent when testing my changes.

For testing, I'd recommend creating an example (see cloudbuild_trigger_filename for the current one for this resource) that sets some/all of the new fields, and then adding it to terraform.yaml. That'll generate a test that sets them. After that, I'd also recommend adding the new fields to the existing (or a new) update test in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/third_party/terraform/tests/resource_cloudbuild_trigger_test.go) so we know they can be updated.

Let me know if you have further questions!

@danawillow - according to the original issue (#3576) I'm still missing a few attributes in this PR but I wanted to take this opportunity to do a quick gut check on the iteration thus far. Your feedback was great, and I did my best to implement some of it (but not as thorough as I would like it to be, ie more tests, fields, etc. - will you comment on that).

I did have a strange issue when running bundle exec ... on the magic modules repo - but it ultimately generated the files I cared about in the terraform modules and I was able to test it.
More specifically in magic modules I executed:

bundle exec compiler -p products/cloudbuild -v "ga" -e terraform -o "$GOPATH/src/github.com/terraform-providers/terraform-provider-google"

and would get errors like:

bundler: failed to load command: compiler (compiler) 
Errno::EMFILE: Too many open files @ rb_sysopen - third_party/terraform/tests/data_source_google_compute_instance_serial_port_test.go 
   /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/fileutils.rb:1385:in `initialize'  
...

Then I'd go to the terraform directory in my $GOPATH and run:

export GOOGLE_CREDENTIALS=myfile.json
export GOOGLE_PROJECT=myproject
export GOOGLE_REGION=us-central1
export GOOGLE_ZONE=us-central1-a
make testacc TEST=./google TESTARGS='-run=TestAccCloudBuildTrigger_*'

I have not done the beta yet but will shortly. Hope that wasn't too much - thanks again for the guidance 🙂

danawillow · 2020-07-14T20:44:15Z

Are you running on a mac? If so I fix that error by running ulimit -n 1024

modular-magician · 2020-07-17T01:48:22Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 807 insertions(+))
Terraform Beta: Diff ( 4 files changed, 807 insertions(+))
Ansible: Diff ( 2 files changed, 557 insertions(+))
TF Conversion: Diff ( 1 file changed, 286 insertions(+))
TF OiCS: Diff ( 4 files changed, 130 insertions(+))
Inspec: Diff ( 9 files changed, 249 insertions(+))

danawillow · 2020-07-25T00:17:28Z

Hey, just wanted to pop in and let you know I haven't forgotten about this! I've had a bunch of other things on my plate, but I should be able to look at this again next week.

danawillow · 2020-07-30T16:55:06Z

templates/terraform/examples/cloudbuild_trigger_build.tf.erb

@@ -0,0 +1,29 @@
+resource "google_cloudbuild_trigger" "<%= ctx[:primary_resource_id] %>" {


The test generated from this example fails:

------- Stdout: ------- === RUN TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample === PAUSE TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample === CONT TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample: testing.go:674: Step 0 error: config is invalid: "github": one of `github,trigger_template` must be specified --- FAIL: TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample (0.46s) FAIL

My guess is even when that is fixed, the test will probably still fail because some of the references in this example are to things that don't exist. https://github.com/GoogleCloudPlatform/magic-modules#testing-your-changes talks about testing changes. We can run the tests for you, but it's obviously faster if you can iterate on them yourself before sending it back :)

Oh this is strange I thought I did run all the tests and confirm they passed before making PRs ready. Will look at this again this evening 🤔

@danawillow okay I see it was because I was testing with the general Terraform provider, when I ran the test on the Terraform beta provider I got the same error. I made a the correction to the test example here and now all tests are passing again 🤷

Anything else to look at?

Want to note I still need to fill out some of the specific sub resources mentioned in the original ticket. Just want to ensure things are looking good before I proceed and knock the rest out (including a test that includes a comprehensive build example)

modular-magician · 2020-08-03T01:29:55Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 817 insertions(+))
Terraform Beta: Diff ( 4 files changed, 817 insertions(+))
Ansible: Diff ( 2 files changed, 557 insertions(+))
TF Conversion: Diff ( 1 file changed, 286 insertions(+))
TF OiCS: Diff ( 4 files changed, 135 insertions(+))
Inspec: Diff ( 9 files changed, 249 insertions(+))

danawillow

LGTM, just one last thing I noticed. I'm happy to have you fix this and then merge this in and do the rest in a follow up (that way people can start using them sooner), but if you'd prefer to do the rest in this PR that's fine too.

danawillow · 2020-08-03T18:59:52Z

products/cloudbuild/api.yaml

+                  Location of the source in an archive file in Google Cloud Storage.
+                properties:
+                  - !ruby/object:Api::Type::String
+                    name: 'bucket'


Should this and object be required?

Fair point - wasn't explicitly called out in REST reference which may explain why I missed it.

tanjinP · 2020-08-05T00:31:58Z

LGTM, just one last thing I noticed. I'm happy to have you fix this and then merge this in and do the rest in a follow up (that way people can start using them sooner), but if you'd prefer to do the rest in this PR that's fine too.

Just ran tests on latest commit and confirmed it's passing.

Hmmm, in that case let's go ahead and get this merged in and I can make a follow up PR with the rest of the items. Am generally a fan of the iterative approach and this seems a good a time as any 🙂

modular-magician · 2020-08-05T00:33:27Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 821 insertions(+))
Terraform Beta: Diff ( 4 files changed, 821 insertions(+))
Ansible: Diff ( 2 files changed, 560 insertions(+))
TF Conversion: Diff ( 1 file changed, 286 insertions(+))
TF OiCS: Diff ( 4 files changed, 135 insertions(+))
Inspec: Diff ( 9 files changed, 249 insertions(+))

tanjinP added 2 commits June 10, 2020 19:12

adding tags

0c5cfb4

adding the build properties (minus artifacts and options)

82c5986

googlebot added the cla: yes label Jun 11, 2020

modular-magician requested a review from SirGitsalot June 11, 2020 12:55

danawillow requested review from danawillow and removed request for SirGitsalot June 16, 2020 18:45

danawillow suggested changes Jun 17, 2020

View reviewed changes

Merge remote-tracking branch 'google/master' into 3576-missing-cloud-…

c5fe3be

…build-properties

Merge remote-tracking branch 'google/master' into 3576-missing-cloud-…

c19718d

…build-properties

adding example + test

fa67b48

documentation and rename secrets to secret

67d19d9

tanjinP marked this pull request as ready for review July 8, 2020 15:11

tanjinP requested a review from danawillow July 8, 2020 15:11

removing exactly_one_of

75a8217

danawillow reviewed Jul 30, 2020

View reviewed changes

correcting test example

2caf210

danawillow reviewed Aug 3, 2020

View reviewed changes

making bucket and object for storage source required

538232b

danawillow approved these changes Aug 5, 2020

View reviewed changes

danawillow merged commit 27d238c into GoogleCloudPlatform:master Aug 5, 2020

AlfatahB mentioned this pull request Dec 15, 2022

google_cloudbuild_trigger is missing some parameters hashicorp/terraform-provider-google#5108

Closed

sanghaniJ mentioned this pull request Dec 20, 2022

Cloud Build - no Build Secret field but secret_env is available in steps hashicorp/terraform-provider-google#4844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Missing Cloud Build Attributes #3627

Adding Missing Cloud Build Attributes #3627

tanjinP commented Jun 11, 2020 •

edited by danawillow

Loading

modular-magician commented Jun 11, 2020

tanjinP commented Jun 11, 2020

modular-magician commented Jun 11, 2020

modular-magician commented Jun 11, 2020

tanjinP commented Jun 16, 2020

danawillow left a comment

danawillow Jun 17, 2020

tanjinP Jul 8, 2020

danawillow Jul 14, 2020

tanjinP Jul 17, 2020

danawillow commented Jul 7, 2020

tanjinP commented Jul 7, 2020

modular-magician commented Jul 7, 2020

modular-magician commented Jul 8, 2020

modular-magician commented Jul 8, 2020

tanjinP commented Jul 8, 2020

danawillow commented Jul 14, 2020

modular-magician commented Jul 17, 2020

danawillow commented Jul 25, 2020

danawillow Jul 30, 2020

tanjinP Jul 30, 2020

tanjinP Aug 3, 2020

tanjinP Aug 3, 2020

modular-magician commented Aug 3, 2020

danawillow left a comment

danawillow Aug 3, 2020

tanjinP Aug 5, 2020

tanjinP commented Aug 5, 2020

modular-magician commented Aug 5, 2020

		@@ -0,0 +1,29 @@
		resource "google_cloudbuild_trigger" "<%= ctx[:primary_resource_id] %>" {

Adding Missing Cloud Build Attributes #3627

Adding Missing Cloud Build Attributes #3627

Conversation

tanjinP commented Jun 11, 2020 • edited by danawillow Loading

modular-magician commented Jun 11, 2020

tanjinP commented Jun 11, 2020

modular-magician commented Jun 11, 2020

Diff report:

modular-magician commented Jun 11, 2020

tanjinP commented Jun 16, 2020

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow commented Jul 7, 2020

tanjinP commented Jul 7, 2020

modular-magician commented Jul 7, 2020

Diff report:

modular-magician commented Jul 8, 2020

Diff report:

modular-magician commented Jul 8, 2020

Diff report:

tanjinP commented Jul 8, 2020

danawillow commented Jul 14, 2020

modular-magician commented Jul 17, 2020

Diff report:

danawillow commented Jul 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Aug 3, 2020

Diff report:

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanjinP commented Aug 5, 2020

modular-magician commented Aug 5, 2020

Diff report:

tanjinP commented Jun 11, 2020 •

edited by danawillow

Loading