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

feat: expose storage class for IPFS containers #148

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

nathanielc
Copy link
Collaborator

No description provided.

Copy link
Contributor

@3benbox 3benbox left a comment

Choose a reason for hiding this comment

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

A question about defaults

@@ -110,6 +117,7 @@ impl Default for RustIpfsConfig {
memory: Some(Quantity("512Mi".to_owned())),
storage: Quantity("1Gi".to_owned()),
},
storage_class: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure exactly how this will evaluate with Rust.

If there is no storageClassName specified, then the PVC should not set one, not even an empty string, so that the default storageClassName is applied.

https://kubernetes.io/docs/concepts/storage/persistent-volumes/#class-1

Copy link
Collaborator Author

@nathanielc nathanielc Feb 13, 2024

Choose a reason for hiding this comment

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

That is what is happening, if no storage class is specified in the spec then its not set on the PVC spec itself. Rust does this via the Option<String> type, where None means its not set and Some("some_storage_class_name") is set. This also means that None is different than Some("") i.e. the empty string. We do not use Some("") in these code paths. We either set it to None or to Some with a non empty string.

Copy link
Contributor

@3benbox 3benbox left a comment

Choose a reason for hiding this comment

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

LGTM

@nathanielc nathanielc added this pull request to the merge queue Feb 13, 2024
Merged via the queue into main with commit 82e6fe3 Feb 13, 2024
5 checks passed
@nathanielc nathanielc deleted the feat/ipfs-storage-class branch February 13, 2024 18:53
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