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

Set empty value for NodeSelector #106

Closed
shubhamcoc opened this issue Apr 10, 2023 · 9 comments · Fixed by #107
Closed

Set empty value for NodeSelector #106

shubhamcoc opened this issue Apr 10, 2023 · 9 comments · Fixed by #107

Comments

@shubhamcoc
Copy link
Contributor

Is it possible to set empty value for NodeSelector?
Use case: If we want to generate helm chart with default values as empty for NodeSelector.

Let me know, if it is possible currently.

@arttor
Copy link
Owner

arttor commented Apr 10, 2023

Hi, @shubhamcoc, thank you for submitting the issue. I forgot to mention it in the documentation but i try to follow these rules for introducing new issues:

  • backward compatibility - don't change the structure of already templated fields
  • enforce Helm best practice and security
  • don't add fields that are not presented in the source manifests. Users can shape helmify output only by modifying source, or using helmify flags. If we add non-existing fields and values to charts, users won't be able to remove them by editing source manifests if needed.

@arttor
Copy link
Owner

arttor commented Apr 10, 2023

Can your problem be solved by adding nodeSelector to the source manifest?

We can add flag for empty nodeSelector but this approach does not scales well. It is hard to manage a big amount of flags to control all spec fields in k8s objects individually. In this case, it makes sense to start working on a detailed config like proposed in #59

@shubhamcoc
Copy link
Contributor Author

Hi @arttor, If we add to the source manifest, and the nodeSelector is empty valued. helmify is omitting it in the generated charts and value.yaml file.

@arttor
Copy link
Owner

arttor commented Apr 10, 2023

can you please give more info about your use-case? For example, if you are using kubebuilder project with kustomize, then nodeSelector maybe be somehow supported in your kustomize config.

We can introduce a flag adding nodeSelector to a chart (disabled by default) but in this case, it makes sense to cover other pod scheduling features like affinity. What do you think?

@shubhamcoc
Copy link
Contributor Author

Hi @arttor, the nodeSelector field is present in the source manifest. It is getting omitted because it is empty. (For example: nodeSelector: {}), now here the map len is zero but the field is present. I hope it clarifies your doubt.

@arttor
Copy link
Owner

arttor commented Apr 10, 2023

@shubhamcoc in this case we can replace if len(spec.NodeSelector) != 0 { to if spec.NodeSelector != nil in pkg/processor/pod/pod.go:76 and it should solve the issue. Is it correct?

@shubhamcoc
Copy link
Contributor Author

@arttor, Yes you are right. Let me change in the PR as well.

@shubhamcoc
Copy link
Contributor Author

@arttor, done with the changes. Can you review it once? Thanks!!

@arttor
Copy link
Owner

arttor commented Apr 10, 2023

@shubhamcoc thank you for your contribution. changes are available in v0.4.1 release.

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 a pull request may close this issue.

2 participants