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

[bitnami/template] Add scripts and instructions for generating new charts from the template #25968

Closed
wants to merge 7 commits into from

Conversation

dan-hill2802
Copy link
Contributor

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 and docker

Checklist

…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>
@javsalgar javsalgar added in-progress verify Execute verification workflow for these changes labels May 20, 2024
@github-actions github-actions bot removed the triage Triage is needed label May 20, 2024
@github-actions github-actions bot removed the request for review from javsalgar May 20, 2024 08:18
@github-actions github-actions bot requested a review from andresbono May 20, 2024 08:18
Copy link
Member

@andresbono andresbono 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 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!

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Signed-off-by: Daniel Hill <dan@mamu.co>
Signed-off-by: Daniel Hill <dan@mamu.co>
@dan-hill2802
Copy link
Contributor Author

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!

All issues now addressed.

@andresbono
Copy link
Member

Thank you for addressing the minor suggestions, @dan-hill2802. Having another look to the PR, I wonder if 3 shell scripts plus a Makefile isn't too much for the relatively simple task we want to accomplish with them. I think it would suffice with a single shell script under ./template/. The feature is not going to be part of any CI/CD action for now. What do you think?

@dan-hill2802
Copy link
Contributor Author

Thank you for addressing the minor suggestions, @dan-hill2802. Having another look to the PR, I wonder if 3 shell scripts plus a Makefile isn't too much for the relatively simple task we want to accomplish with them. I think it would suffice with a single shell script under ./template/. The feature is not going to be part of any CI/CD action for now. What do you think?

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: ./template/run.sh new-chart $$chart_name and ./template/run.sh render-template $$chart_name? If so, then that makes sense and I can make that change.

@andresbono
Copy link
Member

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:

  • The template dir is not supposed to be converted into a chart directly for basic scaffolding, (kind of what helm create does). We can think of it more as a library where you pick the specific templates you need after the architecture of the chart is defined.
  • It will increase the maintenance load, more logic to keep up-to-date for something that is not going to add much value.

I hope you understand, we really appreciate your efforts on putting this PR together. Do not hesitate to submit new contributions!!

@dan-hill2802
Copy link
Contributor Author

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.
I will maintain my fork with this feature. If it is deemed useful in the future it should still be able to merge, as I don't anticipate many conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Want a tool to generate helm application templates with one click
3 participants