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]: Add resource_storage_transfer_job #1069

Conversation

akramhussein
Copy link
Contributor


[all]

[terraform]

Add resource_storage_transfer_job

[terraform-beta]

[ansible]

[inspec]

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@akramhussein
Copy link
Contributor Author

Do I need to add the resource to provider.go.erb here?

@rileykarson
Copy link
Member

Yep! I haven't looked over this PR yet but you'll need to do that here as well.

@akramhussein
Copy link
Contributor Author

Done!

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.

Real quick pass of formatting/nitpicks before we get started, I haven't looked at the actual contents in any depth yet.

@akramhussein akramhussein force-pushed the tf-resource_google_storage_transfer_job branch from 3683de4 to 0f3eaab Compare December 18, 2018 00:29
@akramhussein
Copy link
Contributor Author

Have also rebased on master and squashed this PR

@akramhussein
Copy link
Contributor Author

I signed it!

@akramhussein akramhussein force-pushed the tf-resource_google_storage_transfer_job branch 2 times, most recently from 0b29b89 to d70b15a Compare December 18, 2018 11:13
@akramhussein
Copy link
Contributor Author

Hi @rileykarson - I've rebased on master and also updated the authorship on this commit to match that for the signed CLA. Hopefully it should pick it up - it's done under @project-sapiens.com

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.

Looking great! Most if not all of my comments are just consistency/formatting, sorry for the churn. I tried to provide examples in a few places where the change seemed kind of silly.

For expand/flattens, I only commented on TransferSpecs so far but I think the comment applies to a couple of different fields.

@rileykarson rileykarson self-assigned this Dec 18, 2018
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.

Also, I saw these errors when trying to generate downstreams (we need the build to pass to generate them):

google-beta/resource_storage_transfer_job.go:34:19: undefined: StringLenBetween
google-beta/validation.go:198:16: undefined: time

@akramhussein
Copy link
Contributor Author

akramhussein commented Dec 18, 2018

Hi @rileykarson I'll work through these changes as individual commits and then rebase/squash.

Can I ask, what command are you using to compile and generate downstreams?

@rileykarson
Copy link
Member

rileykarson commented Dec 18, 2018 via email

@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#256
depends: hashicorp/terraform-provider-google#2707

@akramhussein akramhussein force-pushed the tf-resource_google_storage_transfer_job branch 2 times, most recently from 4a57c44 to abc5a66 Compare December 19, 2018 23:14
@akramhussein
Copy link
Contributor Author

Hi @rileykarson I believe I've now addressed all the comments except for the update unit test.
Would you be able to have a quick pass and let me know if everything looks good? Thanks for being patient! Nearly there (never written go before!)

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.

This is looking great! Thanks for your patience with leaning Go + working with the weird unwritten style we tend to follow for our resources at the same time 🙂

I've kicked off a Magician run to regenerate the downstreams in the provider repos, I've left a couple small comments in the meantime if you don't mind looking over those while adding an update test.


Quick update from the run: looks like we're getting these errors from the linters (they can be ran locally with make lint in a provider repo)

==> Checking source code against linters...
google/resource_storage_transfer_job.go:1::warning: file is not gofmted with -s (gofmt)
google/resource_storage_transfer_job.go:595:6:warning: func flattenAwsAccessKeys is unused (U1000) (unused)
make: *** [lint] Error 1
The command "make lint" exited with 2.

I got a quota error running the test, but that's likely just me needing to submit a quota request. I'll debug that tomorrow.

$ terraform import google_storage_transfer_job.nightly-backup-transfer-job my-project-1asd32/8422144862922355674
```

Note that when importing a Transfer Job (and only when importing), the Compute API needs to be enabled - you'll see an error with a link to the enablement page if it is not.
Copy link
Member

Choose a reason for hiding this comment

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

If you end up encountering this again, I would include this; otherwise I'd slightly prefer omitting it until it's repro'ed again. My expectation is that it was a transient error because an error from the Compute API doesn't make much sense here?

@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 9c6b135) have been included in your existing downstream PRs.

@akramhussein
Copy link
Contributor Author

akramhussein commented Dec 20, 2018

Fixed the linting issue. Unfortunately during testing i hit a rate limit:

make testacc TEST=./google TESTARGS='-run=TestAccStorageTransferJob_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccStorageTransferJob_basic -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccStorageTransferJob_basic
=== PAUSE TestAccStorageTransferJob_basic
=== CONT  TestAccStorageTransferJob_basic
--- FAIL: TestAccStorageTransferJob_basic (239.03s)
    testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:

        * google_storage_transfer_job.transfer_job: 1 error(s) occurred:

        * google_storage_transfer_job.transfer_job: Error reading Transfer Job "transferJobs/17748877939099650077": googleapi: Error 429: Quota exceeded for quota group 'ReadOnlyGroup' and limit 'USER-100s' of service 'storagetransfer.googleapis.com' for consumer 'project_number:278407580213'., rateLimitExceeded
FAIL
FAIL	github.com/terraform-providers/terraform-provider-google/google	239.079s
make: *** [testacc] Error 1

Update

Oh my...when I changed the id behaviour, I over zealously copied & pasted and end up with recursion on the read method 🤦‍♂️ . Fixed

@akramhussein
Copy link
Contributor Author

Removed the line about requiring the Compute API - I did some testing and couldn't replicate it.

@akramhussein
Copy link
Contributor Author

Hi @rileykarson - I’ve added 2 more steps that 1) change data sink name; then 2) change the data source name. I’ve re-used the same template schema - didn’t see benefit of duplicating it.

I’ve tested these against our GCP account and all works. Think that's it?

@akramhussein akramhussein force-pushed the tf-resource_google_storage_transfer_job branch from 980f06c to 31ef38c Compare December 20, 2018 14:13
@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 22e2b94) have been included in your existing downstream PRs.

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.

Just a couple minor config nits + some comments about id left I think! Inside the import tests, Terraform effectively performs an import using the id; we expect an import shape of my-project-1asd32/8422144862922355674 but Terraform sends the id of transferJobs/8422144862922355674. This leads to a test failure when Terraform tries to treat transferJobs as the project id.

Do you mind changing the id everywhere to be {{projectId}}/{{name}} (w/o prefix) so that it's a valid import id?

@googlebot
Copy link

CLAs look good, thanks!

@akramhussein akramhussein force-pushed the tf-resource_google_storage_transfer_job branch 2 times, most recently from 6d20acb to 2c12012 Compare December 20, 2018 20:57
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@akramhussein
Copy link
Contributor Author

I've addressed all the points - can you just double check the setId calls?

The CLA was also signed and did go green but I think I pushed some intermediate commits before changing authorship and it got confused.

@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit a3097f8) have been included in your existing downstream PRs.

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.

I have a few more id/test comments, nothing too big!

@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 729eaf7) have been included in your existing downstream PRs.

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.

Ah, looks like I forgot to comment something - sorry for the extra round trip!

@akramhussein
Copy link
Contributor Author

Updated the tests and they pass now.

@akramhussein akramhussein force-pushed the tf-resource_google_storage_transfer_job branch from 5775871 to 1b6482e Compare December 20, 2018 23:13
@googlebot
Copy link

CLAs look good, thanks!

@akramhussein akramhussein force-pushed the tf-resource_google_storage_transfer_job branch from 1b6482e to 8852674 Compare December 20, 2018 23:15
@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit c70dccc) have been included in your existing downstream PRs.

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.

LGTM! Thanks for your contribution, and for your patience going through review on this change. I'll merge the downstreams & then use the Magician to merge upstream here.

Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.
@akramhussein
Copy link
Contributor Author

🎉 thanks!

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.

4 participants