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

Feature/go118 dependency update #1811

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

dbarentine
Copy link

@dbarentine dbarentine commented Jun 29, 2022

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

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]>
@dbarentine
Copy link
Author

@kensipe @alenkacz @zen-dog @jbarrick-mesosphere Any possibility of getting a review/merge of this?

@kensipe
Copy link
Member

kensipe commented Jul 19, 2022

@dbarentine I will look today.

Copy link
Member

@kensipe kensipe left a 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
Copy link
Member

Choose a reason for hiding this comment

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

Was this change intentional?

Copy link
Author

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")
Copy link
Member

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

Copy link
Author

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`
Copy link
Member

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?

Copy link
Author

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(
Copy link
Member

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

Copy link
Author

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)
Copy link
Member

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

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants