-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: Added aws_api_gateway_api_key value attribute #9462
Conversation
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.
Hi @Ninir
Thanks for the work here - I am slightly worried that we are sending a value as part of the API request without it being able to be set by the user - thoughts on this?
Also, can this value be updated? Or will a new value cause a new resource?
Paul
@@ -71,6 +76,7 @@ func resourceAwsApiGatewayApiKeyCreate(d *schema.ResourceData, meta interface{}) | |||
Description: aws.String(d.Get("description").(string)), | |||
Enabled: aws.Bool(d.Get("enabled").(bool)), | |||
StageKeys: expandApiGatewayStageKeys(d), | |||
Value: aws.String(d.Get("value").(string)), |
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.
IF this value is Computed: true and then nothing else, a user cannot set it
therefore this will fail - does the value
need to be optional as well?
@@ -86,7 +92,8 @@ func resourceAwsApiGatewayApiKeyRead(d *schema.ResourceData, meta interface{}) e | |||
log.Printf("[DEBUG] Reading API Gateway API Key: %s", d.Id()) | |||
|
|||
apiKey, err := conn.GetApiKey(&apigateway.GetApiKeyInput{ | |||
ApiKey: aws.String(d.Id()), | |||
ApiKey: aws.String(d.Id()), | |||
IncludeValue: aws.Bool(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.
I think this should be IncludeValue true only if a value is set - thoughts?
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.
Well, the value is always set, either by AWS itself (generated), or the user (custom).
I guess it is just good to always expose it :)
978bee5
to
486b02a
Compare
024bd60
to
52b4c5c
Compare
Hi @Ninir If you can, please rebase this and I can get this tested and merged finally :) Thanks Paul |
@stack72 Here it is! Currently running the acceptance test (just in case). EDIT: $ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayApiKey_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/01/09 14:27:30 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayApiKey_basic -timeout 120m
=== RUN TestAccAWSAPIGatewayApiKey_basic
--- PASS: TestAccAWSAPIGatewayApiKey_basic (22.78s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/aws 22.804s |
Hi @Ninir Is this a sensitive value? i.e. is this something we want added to state? Paul |
We may want to set is as sensitive or hide it from the state, since it acts as an authentication token yes. Will set the attribute in the schema. Thanks for pointing this out! |
abf7ac9
to
8ec2694
Compare
Hi @stack72, The sensitive attribute has been set. Thanks! |
Hi @Ninir If you can rebase this, then we can get this merged - sorry this has taken so long! Paul |
Hi Paul, Just rebased it :) As a note, the "API Gateway" API changed so even if this work will still help previous version of the API Gateways, new API Gateways will require Usage Plans (work already started on my own, PR exposed and soon to be ended) and the related API Keys. Thanks! |
LGTM now thanks :) |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Hi there!
This closes #11105
Relevant Terraform version
Checked against: Terraform v0.7.8-dev (bdb6089+CHANGES)
Acceptance Tests
References
API Reference from the SDK