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

feat: add service account tokens #186

Merged
merged 11 commits into from
Aug 28, 2024
Merged

Conversation

sighphyre
Copy link
Member

Adds service account tokens to the service accounts. This one's a bit more hairy than the last because of sensitivity reasons and how we handle tokens on service accounts for a few reasons:

  1. You can never recover a service account token that you create. It's on you to save it and do something with it.
  2. The description field is unique and required
  3. There's no update available for tokens

Because of these reasons, I've chosen to mark the accessible properties as requiring a plan change. This means that any update to a service account token will trigger a delete and then recreate it from scratch

@sighphyre sighphyre changed the base branch from main to feat/add-service-accounts August 28, 2024 08:20
ServiceAccountId types.Int64 `tfsdk:"service_account_id"`
Secret types.String `tfsdk:"secret"`
Description types.String `tfsdk:"description"`
ExpiresAt types.String `tfsdk:"expires_at"`
Copy link
Member Author

@sighphyre sighphyre Aug 28, 2024

Choose a reason for hiding this comment

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

Notably I've excluded a few fields that I don't think make sense:

  • LastSeen: doesn't seem to be useful for a terraform integration
  • UserId: also doesn't seem to be useful for a terraform integration

This is really just to reduce public dependencies. These can be added later if they're needed but adding them now means making them sticky

Base automatically changed from feat/add-service-accounts to main August 28, 2024 09:02
Copy link
Collaborator

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

Looks great! It seems it has some conflicts with your previous PR but nothing worring

Comment on lines +68 to +70
PlanModifiers: []planmodifier.Int64{
int64planmodifier.RequiresReplace(),
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL "to mark the accessible properties as requiring a plan change. This means that any update to a service account token will trigger a delete and then recreate it from scratch"

Nice! I think this might be handy for #163 as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me too! Thank you ChatGPT cough

Oh cheers, might be valuable to be able to address that issue actually!

})
}

func TestAccServiceAccountTokenResourceUpdatingDescriptionGeneratesNewToken(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤩 nice!

@sighphyre sighphyre force-pushed the feat/add-service-account-tokens branch from a08f8ae to b57c4bb Compare August 28, 2024 09:43
@sighphyre sighphyre merged commit c567260 into main Aug 28, 2024
12 checks passed
@sighphyre sighphyre deleted the feat/add-service-account-tokens branch August 28, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants