Enabling specifying annotations for service accounts in helm chart

[afaytelson]

No description provided.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[lresende]

LGTM, @kevin-bates what is your take on the chart version? particular for the one in dev mode (main branch)? Do we have the release script bumping the version when we perform the release?

[kevin-bates]

The changes look good to me, although we should probably talk about the chart version.

In #1106 (review) @lresende asks:

@kevin-bates what is your take on the chart version? particular for the one in dev mode (main branch)? Do we have the release script bumping the version when we perform the release?

My immediate response is, yes, the release script handles this as of #1102. See:

enterprise_gateway/release.sh

Lines 204 to 208 in 22cd118

	# Update Kubernetes Helm chart and values files (tbump will handle appVersion in Chart.yaml)
	# We need to inject "-" prior to pre-release suffices for 'version:' since it follows strict semantic version rules.
	# For example 3.0.0rc1 -> 3.0.0-rc1
	k8s_version=`echo $RELEASE_VERSION | sed 's/\([0-9]\)\([a-z]\)/\1-\2/'`
	sed -i .bak "s@version: [0-9,\.,a-z]*@version: $k8s_version@g" etc/kubernetes/helm/enterprise-gateway/Chart.yaml

But my question is whether we really need to increment this version string on every change within a release or is just tracking it to the release sufficient? If the former then it would need to ALWAYS be a manual operation and I would argue that we move to a version string that is completely detached from the EG version - although (IMHO) that would be unfortunate. I would prefer that we only update this string on releases and ensure it's a syntactically correct semVer (which was the intention of #1102).

[afaytelson]

@kevin-bates To reiterate your point - I can change back the Chart version to 3.0.0 and we can use the release script for a pre-release. Then once the Chart is ready for a release then it should be updated manually.

[lresende]

@afaytelson Could we leave the version here matching the release version? Thank you.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[kevin-bates]

Then once the Chart is ready for a release then it should be updated manually.

So at that point we'll remove the automation performed in release.sh and it will diverge from the EG release number?

Are charts viewed as their own deliverable packages (separate from EG)? Why would you increment the Chart version within a release when the Chart should not be delivered until the release is complete?