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

Upgrade controller-runtime and controller-gen #464

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Mar 24, 2021

Changes proposed in this PR:

  • Change CRD api-version to v1
  • Update PROJECT version v3-alpha -> v3
  • Update reconciler interface (required by latest controller runtime)
  • Update crds to latest interface

How I've tested this PR: https://app.circleci.com/pipelines/github/hashicorp/consul-helm/2661/workflows/511f1109-3595-4c39-a1c4-e654e0bd3b2e/jobs/9686

How I expect reviewers to test this PR: Code Review

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@thisisnotashwin thisisnotashwin force-pushed the upgrade-sdk-version branch 2 times, most recently from 2627c52 to bd7527f Compare March 24, 2021 21:24
@thisisnotashwin thisisnotashwin marked this pull request as ready for review March 24, 2021 21:26
@thisisnotashwin thisisnotashwin requested review from lkysow, a team and kschoche and removed request for a team March 24, 2021 21:30
@@ -2,30 +2,62 @@ domain: hashicorp.com
layout: go.kubebuilder.io/v2
repo: github.com/hashicorp/consul-k8s
resources:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes in this file are side-effects of updating to Operator SDK 1.15
It changes the version from 3-alpha to 3 are requires additional fields per CRD

Comment on lines +57 to +58
// +kubebuilder:validation:Type=object
// +kubebuilder:validation:Schemaless
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is due to kubernetes-sigs/controller-tools#528 (review) getting merged and now released in latest controller-tools version. It now supports object kind without breaking codegen

Comment on lines +33 to +39
Update(context.Context, client.Object, ...client.UpdateOption) error
// UpdateStatus updates the state of just the object's status.
UpdateStatus(context.Context, runtime.Object, ...client.UpdateOption) error
UpdateStatus(context.Context, client.Object, ...client.UpdateOption) error
// Get retrieves an obj for the given object key from the Kubernetes Cluster.
// obj must be a struct pointer so that obj can be updated with the response
// returned by the Server.
Get(ctx context.Context, key client.ObjectKey, obj runtime.Object) error
Get(ctx context.Context, key client.ObjectKey, obj client.Object) error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because the interface of Reconciler and the controller have been updated to require client.Object and not runtime.Object which required the ConfigEntryResource interface to need an update

Comment on lines -94 to -101
var proxyDefaultsSearch = ` description: Config is an arbitrary map of configuration values used by Connect proxies. Any values that your proxy allows can be configured globally here. Supports JSON config values. See https://www.consul.io/docs/connect/proxies/envoy#configuration-formatting
format: byte
type: string
`
var proxyDefaultsReplace = ` description: Config is an arbitrary map of configuration values used by Connect proxies. Any values that your proxy allows can be configured globally here. Supports JSON config values. See https://www.consul.io/docs/connect/proxies/envoy#configuration-formatting
type: object
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer need to change the type on ProxyDefaults.config because of said PR

- Change CRD api-version to v1
- Update PROJECT to v3 from v3-alpha
- Update reconciler interface
- Update crds to latest interface
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

🦊 🎉

@@ -85,7 +85,7 @@ func TestValidateProxyDefault(t *testing.T) {
require.NoError(t, err)
s := runtime.NewScheme()
s.AddKnownTypes(GroupVersion, &ProxyDefaults{}, &ProxyDefaultsList{})
client := fake.NewFakeClientWithScheme(s, c.existingResources...)
client := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(c.existingResources...).Build()
Copy link
Member

Choose a reason for hiding this comment

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

oh god... the builder pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bahaha!! indeed!!

@@ -86,10 +85,9 @@ type ConfigEntryController struct {
// internal state.
func (r *ConfigEntryController) ReconcileEntry(
crdCtrl Controller,
ctx context.Context,
Copy link
Member

Choose a reason for hiding this comment

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

let's make this the first argument. That seems pretty standard in go.

Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

Looks great!

@thisisnotashwin thisisnotashwin merged commit 06f75dd into master Mar 25, 2021
@thisisnotashwin thisisnotashwin deleted the upgrade-sdk-version branch March 25, 2021 17:22
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants