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

Update charts and docs #11

Merged
merged 5 commits into from
Jul 18, 2024
Merged

Conversation

drew-viles
Copy link
Contributor

This PR is taking the chart and docs updates out of this PR 10 and creating it's own as it's a bit noisy over there.

This will not trigger a new release as it makes sense to do it with the other changes in too - unless there is disagreement there? Happy to do one here but I'll leave that to discussion.

@drew-viles
Copy link
Contributor Author

@spjmurray @yankcrime

I've pulled the chart and doc updates out of the other PR. Once this has gone through I'll rebase the other and we can continue over there ;-)

Let's make this cleaner!

Copy link
Member

@spjmurray spjmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've done too much again 😸

Feel free to ignore most of the comments, I'll swing back around and fix them properly/programatically now you've drawn my attention to them.

@@ -22,14 +22,15 @@ This chart requires the following to be installed on the target cluster first:
```shell
helm repo add jetstack https://charts.jetstack.io
helm repo update
helm install cert-manager jetstack/cert-manager --version v1.10.1 --namespace cert-manager --create-namespace
helm install cert-manager jetstack/cert-manager --version v1.15.1 --namespace cert-manager --create-namespace --set crds.enabled=true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: delete this, we shouldn't be telling users how to install a 3rd part application, and have to update the instructions all the time in different locations, a link to the docs will suffice.

README.md Outdated Show resolved Hide resolved
@@ -67,14 +68,15 @@ There is a top level chart-of-charts that will just install everything as a big
```shell
helm repo add unikorn-cloud-capi https://unikorn-cloud.github.io/helm-cluster-api
helm repo update
helm install unikorn-cloud-capi/cluster-api --version v0.1.1
helm install cluster-api unikorn-cloud-capi/cluster-api --version v0.2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: history repeating itself... we need a way to not have to change versions all over the place and every time something changes. Now asciidoc is good in that it has attributes, so that's a single place, markdown less so. Ideally we'd like the source of truth to be the chart version. So I guess the proposal would be to have a pre-commit hook that looks at charts/*/Chart.yaml and generates a README.md from a template, modifying in place just isn't going to fly given most of these things live in verbatim sections so markup of these things is impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@spjmurray spjmurray Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sed... sounds a bit flaky 🤣 Inspiration of what not to do certainly!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaah yeah, just looked at that script (for probably the first time!).

ok yes, probably some parsing of the Chart.yeaml in the latest release for this would be sufficient then.

charts/cluster-api-provider-openstack/README.md Outdated Show resolved Hide resolved
charts/cluster-api-provider-openstack/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@spjmurray
Copy link
Member

Don't do anything about the version templating by the way, I'll handle that as it applies to the entire organization!

@drew-viles
Copy link
Contributor Author

All updated. I've left the version bump in the docs for the sake of having it updated in this PR but yeah, automation around that would be nice :-)

@drew-viles drew-viles requested a review from spjmurray July 18, 2024 08:37
@@ -2,8 +2,8 @@
CHART_VERSION = v0.2.0
Copy link
Member

@spjmurray spjmurray Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: you need to bump this in the next PR... and obviously change all the versions... again

@@ -49,28 +50,25 @@ spec:
region: en-west-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: ca I fixed, so it resolves automatically and shouldn't be documented, region doesn't exist.

api:
allowList:
- 123.45.67.89
certificateSANs:
- kubernetes.my-domain.com
controlPlane:
version: v1.30.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: we should probably be more opinionated here and just set it at the top level, mixed verison clusters seems like a bad idea.

@spjmurray spjmurray merged commit 9f75951 into unikorn-cloud:main Jul 18, 2024
1 check passed
@drew-viles drew-viles deleted the update-charts branch July 18, 2024 09:29
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.

2 participants