Skip to content

mount-s3-namespace to be conditional #580

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

mount-s3-namespace to be conditional #580

wants to merge 1 commit into from

Conversation

harshashenoy98
Copy link

Issue #, if available:
#579

Description of changes:
Currently, this yaml file gets deployed without condition. We want it to have a condition that by default it can deploy or create the namespace, but if we choose to opt out of it, then don't deploy.

As we use ArgoCD to deploy it, the argo application autocreates the namespace along with certain annotations that we pass from ArgoCD. But this yaml file overrides these annotations causing the s3-csi-controller deployment to have issues

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@harshashenoy98 harshashenoy98 requested a review from a team as a code owner September 11, 2025 12:22
@harshashenoy98 harshashenoy98 mentioned this pull request Sep 11, 2025
Open
muddyfish
muddyfish reviewed Sep 24, 2025
View reviewed changes
Copy link
Contributor

@muddyfish muddyfish left a comment

Choose a reason for hiding this comment

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

LGTM overall, left a small comment regarding the name of the variable

charts/aws-mountpoint-s3-csi-driver/values.yaml

mountpointPod:
namespace: mount-s3
namespaceEnabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change the name to createNamespace - the namespace is still required to use the driver, but the use case for this flag is assuming it's pre-configured

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@muddyfish muddyfish muddyfish left review comments

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@harshashenoy98 @muddyfish