-
Notifications
You must be signed in to change notification settings - Fork 102
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
Feature/go118 dependency update #1811
base: main
Are you sure you want to change the base?
Feature/go118 dependency update #1811
Conversation
Added Azure auth provider plugin Updated for yq v4 Moved MutatingWebhookConfiguration to v1 as v1beta1 is unavailable in k8s v1.22 Defined update flag in every package that go test complained about when it being provided but not defined Dependency update for K8s modules, controller-runtime, KUTTL, and Go to 1.8. Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
…-manager version can't be deployed into K8s 1.22+ Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
Signed-off-by: Dane Barentine <[email protected]>
@kensipe @alenkacz @zen-dog @jbarrick-mesosphere Any possibility of getting a review/merge of this? |
@dbarentine I will look today. |
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.
@dbarentine there appear to be a number of changes not related to 1.18 bump. I do understand that the bump to client-go resulted in some of these changes. Thanks for all the work... can we remove all changes that are not required for the 1.18 and dependency bump? This looks good and lint and test work great.
.goreleaser.yml
Outdated
@@ -116,7 +116,7 @@ snapshot: | |||
|
|||
release: | |||
github: | |||
owner: kudobuilder |
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.
Was this change intentional?
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.
Ah no sorry that's my bad. I updated goreleaser temporarily so I could "release" it into our organization to unblock our Kubernetes upgrade. I'll fix this.
@@ -163,7 +164,8 @@ func main() { | |||
// Add more webhooks below using the above registerWebhook method | |||
|
|||
// Start the KUDO manager | |||
log.Print("Done! Everything is setup, starting KUDO manager now") |
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.
This is a large enough change and specific to 1.18 it seems like a bad idea to inject unrelated updates.. even if small
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.
Similar to the ngrok one the controller runtime changed ports and it wasn't immediately obvious what the problem was. So I had added the port into the startup message to make it clearer where it was running.
I'm happy to back this change out though if you'd prefer.
@@ -86,7 +86,7 @@ These instructions assume you haven't already initialized the cluster with previ | |||
Steps to run the KUDO manager outside the local cluster: | |||
|
|||
1. `make generate-manifests` | |||
1. (separate term) `ngrok http 443` |
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.
was this specific to your env? or is there a reason to updates the dev docs?
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.
Not specific to my environment (or at least I don't think so). It's been a few weeks but I'm pretty sure this change was induced because the controller runtime changed from defaulting to 443 to 9443.
@@ -67,34 +67,33 @@ type Reconciler struct { | |||
// SetupWithManager registers this reconciler with the controller manager | |||
func (r *Reconciler) SetupWithManager( | |||
mgr ctrl.Manager) error { | |||
addOvRelatedInstancesToReconcile := handler.ToRequestsFunc( |
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.
not sure who this is tied to 1.18 bump
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 went back through my notes and I'm honestly not sure what required this change. I don't think it was the 1.18 bump though.
I'm pretty sure it was the bump in the controller-runtime library that caused it. It doesn't look like handler.ToRequestsFunc exists in the bumped version.
@@ -137,7 +136,7 @@ func eventFilter() predicate.Funcs { | |||
} | |||
|
|||
func isForPipePod(e event.DeleteEvent) bool { | |||
return e.Meta.GetAnnotations() != nil && funk.Contains(e.Meta.GetAnnotations(), task.PipePodAnnotation) |
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 are a number of changes that are not tied to 1.18 it appears
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.
The branch name is badly named in this case. I set out to update KUDO so that it would fix the deprecation issue with the Kubernetes API that prevented upgrading to Kubernetes 1.22+. That change itself required an update to the k8s libraries which caused a cascading effect of other dependencies that needed to be updated.
So yes it's much bigger than just a GO 1.18 update. I couldn't find an easy way to isolate just the deprecation fix.
Signed-off-by: Dane Barentine <[email protected]>
What this PR does / why we need it:
Updates:
GO - 1.18
Kuttl - 0.12.1
K8s - 0.20.2
goreleaser - 1.9.2
golangci_lint - 1.45.2
The big thing it does though, is fixes the K8s deprecation issue that prevents installation on K8s 1.22+ clusters.
Supersedes the following PRs:
#1806
#1805
#1799
It also fixes a few issues:
Fixes #1804
Fixes #1807