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 common image tag #102

Closed
wants to merge 1 commit into from

Conversation

Hefeweizen
Copy link
Contributor

I expect to keep all the images in lockstep, so specifying tag versions separately seems an anti-pattern. This change allows them to be easily kept in sync by taking their common config value from one line. It retains the ability to set them separately if needed.

@cdunford
Copy link

One thing you could consider (I've seen done in other charts and have done myself in some) is falling back to use .Chart.AppVersion as the image tag if not overridden. Conceptually it makes sense to me.

@Hefeweizen
Copy link
Contributor Author

One thing you could consider (I've seen done in other charts and have done myself in some) is falling back to use .Chart.AppVersion as the image tag if not overridden. Conceptually it makes sense to me.

Great idea! I like it; let's use preexisting info wherever possible. But, help me talk this through a little bit. The major version is staying the same, and I'm bumping via Update Releases (https://docs.aquasec.com/docs/update-releases). So, as a user should I be modifying Charts.yaml with that specific version (e.g. 5.0.20190), or would I be able to set .chart.appversion in values.yaml. Because, as an end-user, I would only ever expect to update the values.yaml file (chart.yaml is metadata set by the packager, not information I'm picking).

@cdunford
Copy link

Yes you're right; the user wouldn't change Chart.yaml - I think your change still makes sense to allow the user to change all tags at once. I think the benefit of falling back to .Chart.AppVersion is the chart doesn't need to specify a .Values.common.image.tag by default and instead uses the standard AppVersion.

@niso120b
Copy link
Contributor

not relevant based on the helm chart design

@niso120b niso120b closed this Jul 28, 2020
@cdunford
Copy link

@niso120b - do you have more information on what is "not relevant"?

@agilgur5
Copy link
Contributor

agilgur5 commented Aug 5, 2020

Seems like it is pretty common for useful PRs (and issues) to be closed here with little or no comment, as was in my #99 (comment) and in several examples listed in #92

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.

4 participants