-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
[bitnami/template] Add scripts and instructions for generating new charts from the template #25968
Conversation
…holders Signed-off-by: Daniel H <daniel.h@data-edge.co.uk>
Signed-off-by: Daniel H <daniel.h@data-edge.co.uk>
…arts from the template This change was inspired by bitnami#20350 (comment) A docker image is used to execute the scripts to provide conistency between user OS. Signed-off-by: Daniel H <daniel.h@data-edge.co.uk>
Signed-off-by: Daniel H <daniel.h@data-edge.co.uk>
MD041 requires the first line to begin with `#`, but this file begins with the logo and badges Signed-off-by: Daniel H <daniel.h@data-edge.co.uk>
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.
Thank you for your contribution! I have just added suggestions for minor typo fixes to be addressed. Could you please check them?
We will give a more thorough review on a second round of review.
Thank you so much again!
Signed-off-by: Daniel Hill <dan@mamu.co>
All issues now addressed. |
Thank you for addressing the minor suggestions, @dan-hill2802. Having another look to the PR, I wonder if 3 shell scripts plus a |
Hi @andresbono, just to confirm I understand, are you suggesting refactoring the three scripts into 3 separate functions within one script file then executed from make like this: |
Hi again @dan-hill2802, after gathering more feedback from others teammates about this feature and a thorough discussion, even simplifying the logic as I'm suggesting, we consider the PR is not going to add much value in the end and we will not merge it. These are some of the main reasons:
I hope you understand, we really appreciate your efforts on putting this PR together. Do not hesitate to submit new contributions!! |
Thanks for letting me know @andresbono. It's a little disappointing, but I understand your points and agree that it can never be an automated process, as creating a chart requires many architectural decisions. The goal was mainly to reduce the effort and chance of error while copying the template and replacing the placeholders. |
Description of the change
Add a means to automate much of the effort in creating new charts from the template chart
Benefits
Reduced effort in creating new charts. More consistency in new charts.
Possible drawbacks
Feature should work on Linux and Mac operating systems, but not Windows. Windows users should use WSL (Windows Subsystem Linux) or a VM.
Applicable issues
Additional information
Requires
make
anddocker
Checklist