-
Notifications
You must be signed in to change notification settings - Fork 326
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
config entry: add validate_clusters to mesh config entry #4256
Conversation
dc5a8ae
to
4815bf1
Compare
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 pending api
bump
8a4c5c2
to
78c36f3
Compare
@@ -254,7 +254,7 @@ ifeq (, $(shell which controller-gen)) | |||
CONTROLLER_GEN_TMP_DIR=$$(mktemp -d) ;\ | |||
cd $$CONTROLLER_GEN_TMP_DIR ;\ |
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.
It turned out that when I ran make ctrl-generate and make ctrl-manifests probably one or both of those commands errored because I couldn't use the CRD when installing. I found via github issues that older versions of controller-gen can panic so bumped it here
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.
Sounds reasonable to bump this. ❓ when was the fix introduced? It looks like maybe controller-gen
is intended for compatibility with a specific k8s.io version, so we may want to either upgrade to an earlier fix version, or bump our k8s.io dependencies as well: https://github.com/kubernetes-sigs/controller-tools/tree/main?tab=readme-ov-file#compatibility
(I’d lean toward bumping k8s.io but that could lead to a cascading bump of controller-runtime
and other deps, which we’d ideally do with ample time so we aren’t crunched running regression tests to vet them)
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 see evidence in the convo here that controller-gen 0.14 could work, rather than bumping all the way to 0.16, do you think that's preferable, and we reduce the risk of things being super out of date?
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 I feel like the lowest viable / other-deps-aligned bump probably reduces risk now, then a fast follow ticket to get our dependencies updated fully in the next release (aligning w/ our K8s 1.30 support). WDYT?
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.
Yup that makes sense!
019be9e
to
f088b91
Compare
* bump controller-gen and regenerate CRDs and update description
f088b91
to
e8c5d03
Compare
… consul-k8s/controlplane module to latest version
github.com/hashicorp/consul-k8s/control-plane => ../control-plane | ||
github.com/hashicorp/consul-k8s/version => ../version | ||
) | ||
replace github.com/hashicorp/consul-k8s/version => ../version |
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.
If we don't do this, then acceptance cannot differ for k8s.io versions. I did bump consul-k8s control plane though
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.
Can we not keep the replace for control-plane
so we're always testing current for PRs that change it? Or was this made necessary for now by k8s.io / some other thing I'm maybe overlooking
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.
It's the k8s.io version unfortunately! It will keep trying to pull in k8s.io 0.29.8 if we follow exactly. So we will need a fairly fast follow to actually get acceptance on k8s.io 0.29.8 if we want to bring back this replace.
I think since PSPs don't work after K8s 1.25 so it's mostly a matter of removing that config when bumping k8s.io but I just didn't want to add on to an already hairy yak shave which is why I chose to not do it right before the release.
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.
Got it. I think that fast follow is important here so we can keep regression testing strong before 1.20 and Oct. patches. Thanks for being careful and explaining!
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.
One question but otherwise LGTM. Thanks for making sure this got back to main
promptly 🙏🏻
github.com/hashicorp/consul-k8s/control-plane => ../control-plane | ||
github.com/hashicorp/consul-k8s/version => ../version | ||
) | ||
replace github.com/hashicorp/consul-k8s/version => ../version |
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.
Can we not keep the replace for control-plane
so we're always testing current for PRs that change it? Or was this made necessary for now by k8s.io / some other thing I'm maybe overlooking
Changes proposed in this PR
api/v1.29.4
envoyextensions/v0.7.3
troubleshoot/v0.7.1
How I've tested this PR
How I expect reviewers to test this PR
Checklist