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

Allow emptyDir type volume mounts #6803

Merged
merged 1 commit into from
May 22, 2024
Merged

Allow emptyDir type volume mounts #6803

merged 1 commit into from
May 22, 2024

Conversation

onichandame
Copy link
Contributor

Fixes #6802

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Thank you! It looks good to me, just have one question.

hostPath:
path: {{ .hostPath }}
type: Directory
{{- else }}
emptyDir:
{{- toYaml .emptyDir | nindent 10 }}
Copy link
Member

Choose a reason for hiding this comment

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

I am not too familiar with Helm, but if emptyDir is not specified then will this error? Should we do {{- else if .emptyDir }} or make emptyDir default to {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When emptyDir and all other configuration fields(hostPath etc.) are not set, the generated yaml would be emptyDir: null. I just tested that if such yaml is applied it would be transformed to emptyDir: {} by k8s.

I am not sure if it is the intended behaviour tho. I'd imagine that one of existingClaim|hostPath|emptyDir has to be set. Now it effectively defaults to emptyDir if not set. Would it be more desirable if an error is thrown when none of the fields above is set?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh that makes sense, thank you!

I am not sure either; I could see arguments for either way. Hmm I vote we keep it as is it currently is.

@code-asher code-asher merged commit 4ff509d into coder:main May 22, 2024
9 checks passed
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.

[helm chart] Support emptyDir type volume mounts
2 participants