-
Notifications
You must be signed in to change notification settings - Fork 2.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
Improve the Helm readme #14083
Improve the Helm readme #14083
Conversation
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.
very nice 💯 ✔️
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.
lgtm 👍 should be good to merge once the feedback from @sayboras gets addressed 💯
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.
Great patch, LGTM!
test-me-please |
huh, good call on running full CI @kaworu , I didn't realize that the k8s version constraint might cause problems but it did on GKE: https://jenkins.cilium.io/job/Cilium-PR-K8s-GKE/3320/execution/node/144/log/
|
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.
LGTM. One question below about CRDs.
This should allow external sites to better represent Cilium in artifacts generated from the Helm charts. Signed-off-by: Joe Stringer <joe@cilium.io>
Expand the charts to include other common fields so that external systems can pull & use icons, tags, k8s version support, etc. Furthermore, flesh out the readme so it renders better. While we're at it, add an SVG without the cilium word so that it's more likely to render correctly in a square on external sites. Signed-off-by: Joe Stringer <joe@cilium.io>
These mostly needed '--' at the start for the helm-docs container to pick up the descriptions and render them into the README.md. While we're at it, Remove stuttering via the old go style of typing the variable at the start of the sentence, use full sentences, and remove any comments that were named exactly the same as the variable. Signed-off-by: Joe Stringer <joe@cilium.io>
This was not referenced anywhere else, remove it. Signed-off-by: Joe Stringer <joe@cilium.io>
This option was too specific and doesn't make sense to expose as a user-visible flag in the helm charts. Remove it. Signed-off-by: Joe Stringer <joe@cilium.io>
Artifact Hub suggests that if we add such annotations to our charts, then they will show up in the Artifact Hub UI somewhere: https://artifacthub.io/docs/topics/annotations/helm/ Signed-off-by: Joe Stringer <joe@cilium.io>
v1.10 will need more than the first 3 digits of the version semver, it will need the full $major.$minor. Fix this up. Signed-off-by: Joe Stringer <joe@cilium.io>
2f35329
to
a9cc79a
Compare
Here's the incremental diff for the latest changes:
|
test-me-please |
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.
Looks great, thanks for this 🙏 💯 🚀
@@ -2,355 +2,320 @@ | |||
|
|||
![Version: 1.9.90](https://img.shields.io/badge/Version-1.9.90-informational?style=flat-square) ![AppVersion: 1.9.90](https://img.shields.io/badge/AppVersion-1.9.90-informational?style=flat-square) |
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.
I was wondering what these versions represent? 😕
Shouldn't these be 1.9.0
?
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.
In the tree during v1.10 development, we use the version number v1.9.90 today to denote "a version that may be much greater than v1.9.x but still a bit less than v1.10.0".
install/kubernetes/cilium/Chart.yaml
Outdated
home: https://cilium.io/ | ||
version: 1.9.90 | ||
appVersion: 1.9.90 | ||
description: Helm chart for Cilium | ||
kubeVersion: ">= 1.12.0-0" | ||
icon: https://raw.githubusercontent.com/cilium/cilium/master/Documentation/images/logo-solo.svg |
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.
@joestringer I assume we should make this logo dependent on the branch we are? i.e. if we backport this to 1.9 we need to change cilium/master
to cilium/v1.9
?
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.
Addressed with the top commit.
Only net-next failed, everything else green: |
Signed-off-by: Joe Stringer <joe@cilium.io>
bc6eed4
to
fa4a1dd
Compare
retest-net-next |
This was not backported by #14116, will reset the labels to allow the backport to be done. |
Improve the Helm readme by:
For a preview, see the readme in this branch:
https://github.com/joestringer/cilium/tree/submit/helm-improvements/install/kubernetes/cilium
Also nominating for backport to allow these to propagate to external sites when we spin the next release.