-
Notifications
You must be signed in to change notification settings - Fork 2
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
Switch to using Giant Swarm fork for built images, upgrade to CAPI v1.4.5 #138
Conversation
CHANGELOG.md
Outdated
@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Changed | |||
|
|||
- Replace deprecated kustomize config `patchesStrategicMerge` | |||
- Switch to using Giant Swarm fork for built images |
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.
@jkremser Should we first release the new network policy change?
Waiting until network policy changes for CAPI app are all done and released |
Network policies are released now (#148), so I'll update this PR soon |
helm/cluster-api/values.yaml
Outdated
@@ -8,7 +8,7 @@ images: | |||
name: giantswarm/kubeadm-bootstrap-controller | |||
controlplane: | |||
name: giantswarm/kubeadm-control-plane-controller | |||
tag: v1.4.0 | |||
tag: v1.4.2-gs-c42cc50ef # upstream v1.4.2 + initial CircleCI pipeline for Giant Swarm fork + https://github.com/kubernetes-sigs/cluster-api/pull/8586 (Makefile improvement) |
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.
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.
Done and tested now
hack/fetch-manifest.sh
Outdated
# Image does not have a shell or `cat` installed, so extract the file using busybox | ||
empty_context="$(mktemp -d)" | ||
cat >"${empty_context}/Dockerfile.manifest" <<EOF | ||
FROM docker.io/giantswarm/cluster-api-controller:${version} as src |
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.
Shall this be quay.io/giantswarm/cluster-api-controller
instead of docker.io/giantswarm/cluster-api-controller
? Since we are using quay.io
in kustomization.yaml
.
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.
Our cluster-api
CircleCI config pushes to Quay and AliYun. The mirroring then copies those images to Docker Hub. Docker Hub is our main image registry, so that is the preferred source. Here, it doesn't really matter since they're an exact copy. I don't have a strong opinion.
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.
Since Docker Hub is our main image registry, I would use it here too.
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.
+1 for using Docker Hub.
Okay, people like GitHub releases, so I'm going to try that |
I've tested with GitHub releases and CAPI v1.4.5, so the PR now contains these changes and is ready for review. Please compare to https://github.com/giantswarm/cluster-api/tree/release-1.4 since that's the accompanying branch in our fork. Particulary, |
- target: | ||
kind: CustomResourceDefinition | ||
name: (ipaddressclaims\.ipam|extensionconfigs\.runtime|ipaddresses\.ipam).cluster.x-k8s.io | ||
patch: |- | ||
- op: remove | ||
path: /metadata/creationTimestamp |
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.
is this intended? I remember something breaking without this patch.
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.
Yeah, this is fixed in upstream. Keeping it isn't possible since otherwise kustomize would complain about this field missing (you can't say "idempotently delete this and ignore if not exists").
Indirect part of giantswarm/roadmap#2217 since I have to develop CAPI changes and test them.
This PR switches to using our own fork and images of cluster-api (see new
main
/release-1.4
branches in https://github.com/giantswarm/cluster-api). It's consistent to how we manage our CAPA fork – from which I also took parts of the newer "generate patches" script. Once merged, we can turn off mirroring upstream images (retagger).Checklist