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

Adding long lived SA token option (#917) #923

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

gillcaleb
Copy link
Contributor

Addresses #917

The intent of this PR is to allow for the creation of a long-lived API token for the server SA.

@gillcaleb gillcaleb requested a review from a team July 18, 2023 15:16
@hashicorp-cla
Copy link

hashicorp-cla commented Jul 18, 2023

CLA assistant check
All committers have signed the CLA.

@gillcaleb
Copy link
Contributor Author

It's unclear to my why the license/cla check continues to remain in a pending state as I've gone through the signing process and verified its completion on my end. I've tried re-checking it numerous times but to no avail.

@heatherezell
Copy link

It's unclear to my why the license/cla check continues to remain in a pending state as I've gone through the signing process and verified its completion on my end. I've tried re-checking it numerous times but to no avail.

Usually that happens if your contributions are under a different account, ie, email. For example, I see that @stavvy-cgill is making the comments, but @gillcaleb is the commenter here. Try signing with your @stavvy-cgill account and that should fix it. Thanks!

@stavvy-cgill
Copy link
Contributor

It's unclear to my why the license/cla check continues to remain in a pending state as I've gone through the signing process and verified its completion on my end. I've tried re-checking it numerous times but to no avail.

Usually that happens if your contributions are under a different account, ie, email. For example, I see that @stavvy-cgill is making the comments, but @gillcaleb is the commenter here. Try signing with your @stavvy-cgill account and that should fix it. Thanks!

Thank you!

@gillcaleb
Copy link
Contributor Author

Hi team! Just wanted to check back in to get a sense for when this might get a review. Thanks!

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

Thanks for raising this - I'm ok with adding it but I want to make sure Kubernetes' alternative recommendations are made clear. LGTM once a few naming and documentation comments are addressed. Thanks for including the tests and schema update too!

Chart.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
templates/server-serviceaccount-token.yaml Outdated Show resolved Hide resolved
@gillcaleb
Copy link
Contributor Author

Thanks for raising this - I'm ok with adding it but I want to make sure Kubernetes' alternative recommendations are made clear. LGTM once a few naming and documentation comments are addressed. Thanks for including the tests and schema update too!

Appreciate the feedback! I went ahead and added the suggested changes.

@kaibepperling
Copy link

Thanks for opening the PR @gillcaleb! We ran in to the same thing today and were about to open a PR.
Is anything blocking a release of that change?

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

Sorry for the delay revisiting this - and thanks for addressing the comments. The changes look good to me, except we just need a very mechanical replacement of generateSecret with createSecret in a few places to match up with the latest name in values.yaml.

templates/_helpers.tpl Outdated Show resolved Hide resolved
@gillcaleb gillcaleb requested a review from a team as a code owner August 16, 2023 13:23
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'll wait for the acceptance tests to pass and then merge.

@tomhjp tomhjp merged commit 1e12d49 into hashicorp:main Aug 17, 2023
8 checks passed
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.

6 participants