-
Notifications
You must be signed in to change notification settings - Fork 118
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: Do not use default values in the Kubernetes yamls #941
feat: Do not use default values in the Kubernetes yamls #941
Conversation
Signed-off-by: Akash Nayak <akash19nayak@gmail.com>
Thanks for making a pull request! 😃 |
Signed-off-by: Akash Nayak <akash19nayak@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work Akash! I like it! A few suggestions..
@@ -17,3 +17,6 @@ spec: | |||
produces: | |||
KubernetesYamls: | |||
disabled: false | |||
config: | |||
outputPath: "deploy/cicd/argocd" | |||
useDefaultValuesInYamls: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would setDefaultValuesInYamls
be a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
OutputPath string `yaml:"outputPath"` | ||
IngressName string `yaml:"ingressName"` | ||
OutputPath string `yaml:"outputPath"` | ||
UseDefaultValuesInK8sYamls bool `yaml:"useDefaultValuesInK8sYamls"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use the same name in all configs? like say setDefaultValuesInYamls
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Codecov ReportBase: 15.28% // Head: 15.28% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
=======================================
Coverage 15.28% 15.28%
=======================================
Files 50 50
Lines 4600 4600
=======================================
Hits 703 703
Misses 3721 3721
Partials 176 176
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Signed-off-by: Akash Nayak <akash19nayak@gmail.com>
Thank you very much, Ashok ji! :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to rename the internal variables to be set too.
Signed-off-by: Akash Nayak <akash19nayak@gmail.com>
Yes sure, done. I missed renaming the internal variables while I was doing case-sensitive find and replace. |
Addresses #940
Old deployment yaml
New deployment yaml
I have tested enterprise-app deployment on the OCP cluster with the new
deploy/yamls
files and it's working fine.Signed-off-by: Akash Nayak akash19nayak@gmail.com