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

Add serviceName to NFS StatefulSet #229

Draft
wants to merge 11 commits into
base: release/kubernetes-agent/v1
Choose a base branch
from

Conversation

APErebus
Copy link
Contributor

@APErebus APErebus commented Jul 12, 2024

When using a StatefulSet with a headless service, you are supposed to set the spec.serviceName value. We weren't.

However, once a stateful set is created, this value can't be modified.

Because the serviceName can't be modified after creation (even on an upgrade), we only include the service name if the statefulset doesn't exist, or if the statefulset exists and the serviceName is defined.

Not setting this value isn't a big issue because we don't have multiple replica's of the NFS pods

Copy link

changeset-bot bot commented Jul 12, 2024

🦋 Changeset detected

Latest commit: 2f9fc57

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
kubernetes-agent Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@liam-mackie liam-mackie 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 for adding an install test as well for the current version.
The only thing I'm not certain of is whether this now breaks installation via ArgoCD, since I'm not certain if argo works with lookup functions (see argoproj/argo-cd#5202)

@APErebus APErebus marked this pull request as draft July 31, 2024 01:47
@APErebus
Copy link
Contributor Author

Converting back to draft as we may not want to actually merge this change

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