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

Check for Default Storage Class when storageClassName is empty #250

Merged
merged 4 commits into from
Sep 8, 2020

Conversation

manavellamnimble
Copy link
Contributor

@manavellamnimble manavellamnimble commented Aug 31, 2020

@marccampbell @dexhorthy Good morning! I send you the PR to check for a storage class set as default when storageClassName is empty. I can add the parse function to parse the string "true" to bool, but it would also consider other values as "True", "T", "TRUE"...and I think k8s only expect the value true

The default StorageClass has an annotation storageclass.kubernetes.io/is-default-class set to true. Any other value or absence of the annotation is interpreted as false.

Closes #200

src: https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/#changing-the-default-storageclass (item 2)

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM.

@dexhorthy
Copy link
Member

Hi @manavellamnimble thanks for the PR! I think I'm in favor of only supporting exactly what Kubernetes supports (or what they document to support) -- if adding an annotation value of (string) T or yes causes the kubectl get storageclass output to change, then we should treat that as valid:

NAME             PROVISIONER               AGE
standard         kubernetes.io/gce-pd      1d
gold (default)   kubernetes.io/gce-pd      1d

But if Kubernetes only parses the "true" string for this then we should only allow that. Does that make sense?

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM.

@manavellamnimble
Copy link
Contributor Author

@markpundsack @dexhorthy I've added default messages in case storageClassName is empty and no message is provided.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM.

@markpundsack
Copy link
Contributor

Logic and messaging looks good to me. I haven't validated the code functionality.

@laverya laverya merged commit d2bc571 into replicatedhq:master Sep 8, 2020
@manavellamnimble manavellamnimble deleted the defaultStorageClass branch September 8, 2020 21:05
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.

Analyzer for default storage class?
5 participants