-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
New resource: aws_grafana_workspace_api_key
#25286
New resource: aws_grafana_workspace_api_key
#25286
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.
Great pull request! I would recommend you review Resource Contribution Guide and verify that everything that can be implemented is implemented.
I have suggested some changes.
|
||
func ResourceWorkspaceApiKey() *schema.Resource { | ||
return &schema.Resource{ | ||
Create: resourceWorkspaceApiKeyInsert, |
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.
Method should be named *Create, not *Insert.
Create: resourceWorkspaceApiKeyInsert, | |
Create: resourceWorkspaceApiKeyCreate, |
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.
Changed to Create as suggested
"key_name": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
//Computed: 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.
Please remove left over code.
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.
Changed as suggested
"key_role": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
//Computed: 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.
Please remove left over code.
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.
Changed as suggested
} | ||
} | ||
|
||
func resourceWorkspaceApiKeyInsert(d *schema.ResourceData, meta interface{}) error { |
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.
We prefer to use the standard name of *Create.
func resourceWorkspaceApiKeyInsert(d *schema.ResourceData, meta interface{}) error { | |
func resourceWorkspaceApiKeyCreate(d *schema.ResourceData, meta interface{}) error { |
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.
Changed to Create as suggested
"key_name": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
//Computed: 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.
Please add validation that matches the API documentation.
//Computed: true, | |
ValidateFunc: validation.StringLenBetween(1, 100), |
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.
Changed as suggested
}) | ||
} | ||
|
||
func testAccWorkspaceApiKeyProvider_basic(rName string) 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.
Passing in rName, but it is not used in the test.
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.
Now using rName in Test
} | ||
|
||
func testAccWorkspaceApiKeyProvider_basic(rName string) string { | ||
return fmt.Sprintf(` |
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.
unnecessary use of return fmt.Sprintf(, as no strings are being formatted/interpolated.
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.
removed.
resource.TestCheckResourceAttrSet(resourceName, "key_name"), | ||
resource.TestCheckResourceAttrSet(resourceName, "key_role"), | ||
resource.TestCheckResourceAttrSet(resourceName, "seconds_to_live"), | ||
resource.TestCheckResourceAttrPair(resourceName, "workspace_id", workspaceResourceName, "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.
Please add an evaluation of the requested Key
computed output.
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.
Added as requested.
|
||
In addition to all arguments above, the following attributes are exported: | ||
|
||
* `key` - The key token in JSON format. Use this value as a bearer token to authenticate HTTP requests to the workspace. |
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 do not see how this value is exported as the _, err = conn.CreateWorkspaceApiKey(input)
call is discarding the CreateWorkspaceApiKeyOutput
.
Please verify?
|
||
## Import | ||
|
||
Grafana Workspace API Key cannot be imported. |
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 see no reason that the API Key can not be imported.
Grafana Workspace API Key cannot be imported. | |
Grafana Workspace API Key can be imported using the `id`, e.g., | |
```sh | |
$ terraform import aws_grafana_workspace_api_key.example workspace-id/keyname |
-> Note: The key
attribute cannot be imported as it is a token and is only available at create time.
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.
Changed as Requested.
Please also include a copy/paste of AccTest ouutput, not a screenshot. Thank you! |
aws_grafana_workspace_api_key
d0d15df
to
f495674
Compare
af5be97
to
849759a
Compare
a2b392e
to
aa5c4b2
Compare
68dfbd7
to
73cff8d
Compare
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.
Great PR! Some changes requested in addition to Tyler's first review
}) | ||
} | ||
|
||
func testAccWorkspaceAPIKeyConfig_basic(rName string, apiKey string) 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.
For this test function, 2 parameters are not required, we should be using 1 parameter since only 1 parameter is used in the hcl definition
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.
removed the extra parameter.
Importer: &schema.ResourceImporter{ | ||
State: schema.ImportStatePassthrough, | ||
}, |
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.
Since this resource does not support imports, this block can possibly be removed. A similar resource can be found here: https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/kms/ciphertext.go
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.
Deleted as Requested.
Timeouts: &schema.ResourceTimeout{ | ||
Create: schema.DefaultTimeout(10 * time.Minute), | ||
Delete: schema.DefaultTimeout(10 * time.Minute), | ||
}, |
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.
Timeouts are not used in this resource and this block can be removed.
Timeouts: &schema.ResourceTimeout{ | |
Create: schema.DefaultTimeout(10 * time.Minute), | |
Delete: schema.DefaultTimeout(10 * time.Minute), | |
}, |
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.
Deleted as Requested.
{ | ||
Config: testAccWorkspaceAPIKeyConfig_basic(rName, "test-api-1"), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
resource.TestCheckResourceAttr(resourceName, "key_name", "test-api-1"), |
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.
Instead of hard-coding test-api-1
, we can use rName
.
resource.TestCheckResourceAttr(resourceName, "key_name", "test-api-1"), | |
resource.TestCheckResourceAttr(resourceName, "key_name", rName), |
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.
Done as suggested.
ProviderFactories: acctest.ProviderFactories, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccWorkspaceAPIKeyConfig_basic(rName, "test-api-1"), |
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.
We can remove "test-api-1"
since only rName
is used in the HCL
Config: testAccWorkspaceAPIKeyConfig_basic(rName, "test-api-1"), | |
Config: testAccWorkspaceAPIKeyConfig_basic(rName), |
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.
Done as requested, and used just rName as the random name.
Config: testAccWorkspaceAPIKeyConfig_basic(rName, "test-api-1"), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
resource.TestCheckResourceAttr(resourceName, "key_name", "test-api-1"), | ||
resource.TestCheckResourceAttr(resourceName, "key_role", "EDITIR"), |
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.
This can be changed to use the value that the SDK provides
const (
// RoleAdmin is a Role enum value
RoleAdmin = "ADMIN"
// RoleEditor is a Role enum value
RoleEditor = "EDITOR"
// RoleViewer is a Role enum value
RoleViewer = "VIEWER"
)
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.
Done as requested.
|
||
|
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.
Extra empty lines can be removed
|
||
The following arguments are required: | ||
|
||
* `editor_role_values` - (Required) The editor role values. |
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.
The valid values for Role can be defined here. From the documentation at https://docs.aws.amazon.com/grafana/latest/APIReference/API_CreateWorkspaceApiKey.html, the valid values are VIEWER
|EDITOR
|ADMIN
|
||
* `key` - The key token in JSON format. Use this value as a bearer token to authenticate HTTP requests to the workspace. | ||
|
||
## Import |
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.
Since imports are not supported, this can be removed
a5a3662
to
6b6f3bd
Compare
: |
Is there more to be done on this change? Anything I could help with? We're keen to see this change land and stop managing Grafana API keys outside of Terraform! |
This reverts commit 3fe1fec.
This reverts commit 5ab5cfe.
This reverts commit 246ce9b.
This reverts commit ddaec83.
This reverts commit 340061d.
This reverts commit 493888a.
This reverts commit 8c75558.
This reverts commit 4be49dd.
This reverts commit 00e65e8.
This reverts commit d71728b.
This reverts commit 3e32805.
This reverts commit e10eb39.
This reverts commit dc1c31f.
This reverts commit 662197c.
This reverts commit 6782ffc.
This reverts commit eef5fc6.
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.
LGTM 🚀.
% make testacc TESTARGS='-run=TestAccGrafana_serial/ApiKey' PKG=grafana
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/grafana/... -v -count 1 -parallel 20 -run=TestAccGrafana_serial/ApiKey -timeout 180m
=== RUN TestAccGrafana_serial
=== RUN TestAccGrafana_serial/ApiKey
=== RUN TestAccGrafana_serial/ApiKey/basic
--- PASS: TestAccGrafana_serial (135.92s)
--- PASS: TestAccGrafana_serial/ApiKey (135.92s)
--- PASS: TestAccGrafana_serial/ApiKey/basic (135.92s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/grafana 139.938s
@meetreks Thanks for the contribution 🎉 👏. |
Thanks sir, this is really helpful. |
This functionality has been released in v4.28.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
Closes #25100.
Closes #26374.