🌱 Update Cluster API Visualizer chart to v1.0.0 #7018
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
@Jont828: This issue is currently awaiting triage. If CAPI contributors determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/XS
| @@ -3,9 +3,8 @@ replicas: 1 | |||
| image: | |||
| repository: ghcr.io/jont828 | |||
| name: cluster-api-visualizer | |||
| tag: latest # TODO: do we want to pin this to latest? | |||
There was a problem hiding this comment.
@fabriziopandini One way we can use the latest version of the chart is to pin the tag to latest in the values file. I did notice an issue where we aren't fetching the latest version of the chart if the directory hack/observability/visualizer/charts already exists. If we change the tilt-prepare logic to always fetch the latest version of the Helm chart instead, we could keep it up to date that way.
There was a problem hiding this comment.
I would prefer if tilt-prepare treats the chart as a black box, so let's keep tag: latest in the values file
There was a problem hiding this comment.
Sounds good to me. One thing I want to test on my end is if tilt-prepare will fetch the latest version of the chart if hack/observability/visualizer/charts/cluster-api-visualizer folder exists with an older version.
There was a problem hiding this comment.
I think kustomize does not pull a chart again if a chart already exists locally and it looks like any changes to kustomization.yaml (or values.yaml) does not trigger a re-pull. I hit a similar issue when working with other observability tools.
From Kustomize documentation:
If a chart exists, kustomize will not overwrite it (so to suppress a pull, simply assure the chart is already in your kustomization root). kustomize won't check dates or version numbers or do anything that smells like cache management.
With this it looks like each person might be using a different version of the chart depedning on when they pulled it for the first time.
One way to work around this might be to clean up the charts folder under each of the enabled observability tools (eg: delete hack/observability/visualizer/charts) in tilt-prepare before the kustomize task is run. This should force a re-pull.
cc @killianmuldoon This is similar to the issue we hit with promtail.
There was a problem hiding this comment.
That would add to the set-up time, but we could make it configurable somewhere. Another option - I'll also state this on the other issue - might be to drop the helm charting and install directly from manifests.
There was a problem hiding this comment.
In all other places pinned tags are used.
Probably a separate discussion: Looks like the images for a particular chart version are pinned but our kustomization.yaml for the observability tools do not pin a chart version. For a user, depnending on when they run tilt up for the first time it be pulling down a different version of the chart (and potentially a differente version of the component?). I have observed this locally. I had a discussion with @killianmuldoon about this and I will be openning an issue to pin these versions.
Dropping this here as it seems kinda relevant.
There was a problem hiding this comment.
You could argue that for dev it's fine to just use the latest charts, especially as we don't really do any testing for our tilt dev env with those charts and whatever they publish upstream should be fine for us.
But it's definitely a more controlled way to pin it all, so no objections from my side.
Independent of that, I would definitely prefer if released charts are not using latest themselves internally, as that would mean that a specific Chart version could break retroactively when new versions of the image are released.
There was a problem hiding this comment.
Based on what we saw from the kustomize docs, it looks like there isn't a great way to pull the latest version of the chart. The options I can think of for now are to (1) add a flow for cleaning up existing charts, i.e. have make clean remove the local copy of charts` or (2) modify the tilt-prepare logic to pin it to a version. Do you have any thoughts on how we can proceed?
There was a problem hiding this comment.
I would favor an additional make clean-tilt (exact name TBD) target which deletes the charts. I think considering we're talking about a dev env this should be fine for now.
There was a problem hiding this comment.
Regarding the values.yaml here. I would drop all image related fields so we use them from the chart (then we always take the version that is published in the corresponding Chart version)
|
@CecileRobertMichon What do you think about trying to implement some functionality to automatically keep this up to date with the latest image version? One idea is to change the tilt-prepare logic to always fetch the latest version of the chart. Another idea is to pin the version to latest and override the default appVersion. |
k8s-ci-robot
added
size/M
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@Jont828 What do you think about dropping the first commit and merging the second one? I think they don't depend on each other and I would like to use the new version of the visualizer without having to change my source files :) |
|
Sure I'm happy to merge this first. The reason I rebased it off of the Makefile PR is because people probably won't be able to see the update on the chart until they run |
Jont828
force-pushed
the
visualizer-v1
branch
from
0148a21 to
18235b5
Compare
k8s-ci-robot
added
size/XS
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
Makes sense. Let's go ahead anyway. At least folks following the PRs are aware |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it: Update Cluster API Visualizer chart to v1.0.0
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #