-
Notifications
You must be signed in to change notification settings - Fork 9k
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
Add Grafana service account #38101
base: main
Are you sure you want to change the base?
Add Grafana service account #38101
Conversation
Community NoteVoting for Prioritization
For Submitters
|
Thank you for your contribution! 🚀 Please note that typically Go dependency changes are handled in this repository by dependabot or the maintainers. This is to prevent pull request merge conflicts and further delay reviews of contributions. Remove any changes to the Additional details:
|
Got a few questions for reviewers myself |
@@ -227,3 +227,4 @@ vpc_endpoint_id,VPCEndpointID | |||
vpc_id,VPCID | |||
vpc_security_group_ids,VPCSecurityGroupIDs | |||
weight,Weight | |||
workspace_id,WorkspaceID |
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 this names constant, but it triggers a refactor for all the places where there are string litterals with "workspace_id" https://github.com/hashicorp/terraform-provider-aws/actions/runs/9664687528/job/26659952436?pr=38101
Should I go ahead and use names.AttrWorkspaceID
or should I revert this commit instead?
PreCheck: func() { acctest.PreCheck(ctx, t); acctest.PreCheckPartitionHasService(t, names.Grafana) }, | ||
ErrorCheck: acctest.ErrorCheck(t, grafana.ServiceID), | ||
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, | ||
CheckDestroy: testAccCheckWorkspaceServiceAccountDestroy(ctx), |
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.
CheckDestroy
throws an error that looks like a race condition and I don't know how to fix it.
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
make: Building provider...
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.4 test ./internal/service/grafana/... -v -count 1 -parallel 20 -run='TestAccWorkspaceServiceAccount' -timeout 360m
=== RUN TestAccWorkspaceServiceAccount_basic
=== PAUSE TestAccWorkspaceServiceAccount_basic
=== RUN TestAccWorkspaceServiceAccount_disappears
=== PAUSE TestAccWorkspaceServiceAccount_disappears
=== CONT TestAccWorkspaceServiceAccount_basic
=== CONT TestAccWorkspaceServiceAccount_disappears
--- PASS: TestAccWorkspaceServiceAccount_disappears (339.76s)
=== NAME TestAccWorkspaceServiceAccount_basic
workspace_service_account_test.go:29: Error running post-test destroy, there may be dangling resources: operation error grafana: ListWorkspaceServiceAccounts, https response error StatusCode: 404, RequestID: 108fce02-80f4-49d1-8541-4d78d559a454, ResourceNotFoundException: Workspace with id=g-ca7016d798 of account=339743103717 does not exist.
--- FAIL: TestAccWorkspaceServiceAccount_basic (369.06s)
FAIL
FAIL github.com/hashicorp/terraform-provider-aws/internal/service/grafana 374.937s
FAIL
make: *** [testacc] Error 1
However, the same tests outside with a normal main.tf
and apply/destroy do not generate any error. Any insights on how to move forward?
I think the workspace gets destroyed before the service token is deleted.
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.
used acctest.CheckDestroyNoop
on CheckDestroy
. Happy to revisit if there's another path
3rd question - should the implementation of service account token (which share a lot with this PR) continue with the same PR or be separate and branched out of this one? |
any insights on this @justinretzolk @ewbankkit? |
As service account by itself is not really useful, I merged locally my branch on service account token. Happy to drop this commit if it needs to be submitted separately commit 6b70e6e59b7dc0bd1ac652ce3038413f65c11e8b Author: Rodrigue Koffi <[email protected]> Date: Thu Jun 27 21:55:00 2024 +0200 Add service account token doc commit bd3fad4e84eb75815516cd658dc155ece76f5791 Author: Rodrigue Koffi <[email protected]> Date: Thu Jun 27 16:05:28 2024 +0200 Optimize tests paths commit 560148e983736a2e193442d24782e5c755be2fca Author: Rodrigue Koffi <[email protected]> Date: Thu Jun 27 12:41:41 2024 +0200 Add acceptance testing commit 39090f6e22252ff979405715a6e0f094f383933b Author: Rodrigue Koffi <[email protected]> Date: Thu Jun 27 11:32:00 2024 +0200 Add service account token resource
|
Description
Adds support for Grafana Service Account with Amazon Managed Grafana
Relations
Closes #37645
References
Output from Acceptance Testing