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

Adds storage_hmac_key_resource #3084

Merged
merged 23 commits into from
Feb 13, 2020
Merged

Conversation

geojaz
Copy link
Contributor

@geojaz geojaz commented Feb 6, 2020

I'm working to get the hmac key resource active in the terraform provider.

Note: this code creates the key, but isn't printing the secret right now. I could use a second pair of eyes on this @rileykarson as this is my first magic-modules contrib. Thanks!
Note: At this point I've (manually) cruded and used the key that it returned. Thanks to the help of riley, I think we're pretty much just fine-tuning downstream outputs.

`google_storage_hmac_key`

@modular-magician
Copy link
Collaborator

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. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

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

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 can't see anything out of place. Mind adding an exclude: true in inspec.yaml + ansible.yaml for this resource? Our system gets tripped up sometimes with new resources in existing products and they cause generation to fail.

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, spotted it- the secret is only returned post-create. Commented with more context, and added a suggestion for metadata.

products/storage/api.yaml Show resolved Hide resolved
products/storage/api.yaml Outdated Show resolved Hide resolved
@geojaz
Copy link
Contributor Author

geojaz commented Feb 7, 2020

@rileykarson Thanks so much for the feedback and the hints!
Now it seems to be working as expected. PTAL, and I'll be happy to continue to consider any further suggestions.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 4 files changed, 603 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 4 files changed, 603 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))

@geojaz geojaz changed the title Adds storage_hmac_key_resource [wip] Adds storage_hmac_key_resourc Feb 7, 2020
@geojaz geojaz changed the title Adds storage_hmac_key_resourc Adds storage_hmac_key_resource Feb 7, 2020
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.

Downstream generally looks good, I'd like to verify that it's working with tests. I've got a handful of wording changes / typo fixes, but looking great.

products/storage/terraform.yaml Outdated Show resolved Hide resolved
templates/terraform/post_create/hmac_secret.go.erb Outdated Show resolved Hide resolved
products/storage/terraform.yaml Show resolved Hide resolved
products/storage/api.yaml Outdated Show resolved Hide resolved
products/storage/api.yaml Show resolved Hide resolved
products/storage/api.yaml Outdated Show resolved Hide resolved
products/storage/api.yaml Outdated Show resolved Hide resolved
products/storage/api.yaml Outdated Show resolved Hide resolved
products/storage/terraform.yaml Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 4 files changed, 603 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 4 files changed, 603 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))

1 similar comment
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 4 files changed, 603 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 4 files changed, 603 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 6 files changed, 702 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 702 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 775 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 775 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 774 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 774 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 774 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 774 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 773 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 773 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

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.

Added a few comments for now, going to run tests to verify my assumptions and see if there's anything I missed.

products/storage/api.yaml Outdated Show resolved Hide resolved
products/storage/api.yaml Outdated Show resolved Hide resolved
products/storage/terraform.yaml Show resolved Hide resolved
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.

Here's a handful of formatting / syntax error I ran into while running tests (sorry if they conflict with changes you've since made!)

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 786 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 786 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 783 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 783 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 782 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 782 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 764 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 764 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 120 insertions(+))
TF OiCS: Diff ( 1 file changed, 120 insertions(+))

products/storage/terraform.yaml Show resolved Hide resolved
products/storage/api.yaml Outdated Show resolved Hide resolved
@rileykarson
Copy link
Member

@emilymye: I picked up this PR to get it over the finish line, as the API didn't map to MM all that well. Lots of handholding / custom code to get it to behave correctly. Adding you as reviewer because I made substantial enough changes that I should get a new set of 👀 on it.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 6 files changed, 642 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 642 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 64 insertions(+))
TF OiCS: Diff ( 1 file changed, 64 insertions(+))

1 similar comment
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 6 files changed, 642 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 642 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 64 insertions(+))
TF OiCS: Diff ( 1 file changed, 64 insertions(+))

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 6 files changed, 636 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 636 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 64 insertions(+))
TF OiCS: Diff ( 1 file changed, 64 insertions(+))

@rileykarson rileykarson merged commit 6bf6455 into GoogleCloudPlatform:master Feb 13, 2020
@geojaz geojaz deleted the hmac_key branch February 25, 2020 17:35
nathkn pushed a commit to nathkn/magic-modules that referenced this pull request May 18, 2020
* Adds hmac key

* Fixes up hmacKey resource properties in terraform

* Update products/storage/terraform.yaml

Co-Authored-By: Riley Karson <[email protected]>

* Update products/storage/api.yaml

Co-Authored-By: Riley Karson <[email protected]>

* Adds an example, website links, and several fixes from review

* Adds test and fixes hmac_key id

* tweaking hmac_key example

* Update third_party/terraform/tests/resource_storage_hmac_key_test.go

Co-Authored-By: Riley Karson <[email protected]>

* Update products/storage/api.yaml

Co-Authored-By: Riley Karson <[email protected]>

* Update third_party/terraform/tests/resource_storage_hmac_key_test.go

Co-Authored-By: Riley Karson <[email protected]>

* hmac_key: adds doc warning about secrets, fixes yaml formatting, and improves test

* More fixes for review

* Fixing gofmt

* fix resource name

* more iteration

* More fixes

* RM unused var in hmac_key_test

* hmac_key: Pre-delete and more fixes

* Fixing wrong assumptions

* Edits to support read schema

* Get tests passing, handle deleted items properly

* Sidebar - to _

* Remove the update encoder because the docs are wrong on update format anyways

Co-authored-by: Riley Karson <[email protected]>
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.

5 participants