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.

Sign up for GitHub

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

Jump to bottom

Set aws file secrets as individual files #385

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Set aws file secrets as individual files #385

wants to merge 1 commit into from

Conversation

ryan-dyer-sp
Copy link

what

creates config and credentials as files within .aws instead of mounting entire secret as .aws.

why

This allows for other directories (.aws/cli, .aws/sso) which need to be created/used by the CLI itself for caching to exist.

tests

tested via various configuration of helm values to receive desired result. Running in own environment the case where aws.config is specified. Does assume that config/credentials is never specified along with awsSecretName.

references

closes #380

@ryan-dyer-sp ryan-dyer-sp requested a review from a team as a code owner May 7, 2024 12:30
@GMartinez-Sisti
Copy link
Member

Thanks @ryan-dyer-sp! Can you please provide an example on values.yaml and also bump the chart minor version, I would consider this a feature.

@ryan-dyer-sp
Copy link
Author

ryan-dyer-sp commented May 7, 2024 •
edited
Loading

Thanks @ryan-dyer-sp! Can you please provide an example on values.yaml

IDK what you're asking for here. https://github.com/runatlantis/helm-charts/blob/main/charts/atlantis/values.yaml#L128-L139 isnt changing. Its just mapping the individual elements within the secret to filenames instead of mounting the entire secret as a volume.
One behavior that does change with this is that secrets mounted with subPaths do not auto update (changes to the secret do not get updated within the container). So you do have to restart the atlantis pod in this case. Perhaps I should add the standard checksum annotation/label in order to force a redeploy when the config is updated?
Will update chart.yaml.

Thanks

@ryan-dyer-sp ryan-dyer-sp force-pushed the Issue-380 branch 2 times, most recently from e1cfad6 to e9ee23e Compare May 7, 2024 18:54
@GMartinez-Sisti
Copy link
Member

Hi!

IDK what you're asking for here. https://github.com/runatlantis/helm-charts/blob/main/charts/atlantis/values.yaml#L128-L139 isnt changing. Its just mapping the individual elements within the secret to filenames instead of mounting the entire secret as a volume.

Please check my comment, I'm trying to understand if the values are supposed to change.

One behavior that does change with this is that secrets mounted with subPaths do not auto update (changes to the secret do not get updated within the container). So you do have to restart the atlantis pod in this case. Perhaps I should add the standard checksum annotation/label in order to force a redeploy when the config is updated? Will update chart.yaml.

This is a good idea 👍

@ryan-dyer-sp
Copy link
Author

@GMartinez-Sisti Anything else needed?

@GMartinez-Sisti
Copy link
Member

@GMartinez-Sisti Anything else needed?

I'll look into this soon! 😃

GMartinez-Sisti
GMartinez-Sisti reviewed Jul 18, 2024
View reviewed changes
Copy link
Member

@GMartinez-Sisti GMartinez-Sisti left a comment

Choose a reason for hiding this comment

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

Almost ready to go! Please see the comment below and also bump the minor version for the chart and it will be good to merge. Sorry for taking so long to review!

charts/atlantis/templates/statefulset.yaml Show resolved Hide resolved
@GMartinez-Sisti GMartinez-Sisti added the waiting-on-response Waiting for a response from the user label Jul 18, 2024
@github-actions GitHub Actions
Copy link

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

@github-actions github-actions bot added the Stale label Aug 23, 2024
@ryan-dyer-sp
Copy link
Author

/remove stale

Chart version bumped

@ryan-dyer-sp
Copy link
Author

@GMartinez-Sisti sorry for the delay getting this bumped

@github-actions github-actions bot removed the Stale label Aug 27, 2024
@github-actions GitHub Actions
Copy link

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

@github-actions github-actions bot added the Stale label Sep 27, 2024
@ryan-dyer-sp ryan-dyer-sp force-pushed the Issue-380 branch 6 times, most recently from bf464df to b237e40 Compare October 23, 2024 13:47
@ryan-dyer-sp
Copy link
Author

/remove stale

@ryan-dyer-sp
Copy link
Author

@GMartinez-Sisti re updated. Anything else?

@GMartinez-Sisti
Copy link
Member

@GMartinez-Sisti re updated. Anything else?

Thanks @ryan-dyer-sp! What do you think about adding a unit test to ensure it works as expected and no one breaks it by mistake?

@github-actions github-actions bot removed the Stale label Oct 24, 2024
@github-actions GitHub Actions
Copy link

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

@github-actions github-actions bot added the Stale label Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GMartinez-Sisti GMartinez-Sisti GMartinez-Sisti left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Stale waiting-on-response Waiting for a response from the user
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws cli cache directory unwritable
2 participants
@ryan-dyer-sp @GMartinez-Sisti