Adds Version Check #102

somtochiama · 2020-07-03T15:29:06Z

No description provided.

k8s-ci-robot · 2020-07-03T15:29:13Z

Hi @somtochiama. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

somtochiama · 2020-07-07T08:01:15Z

/assign @justinsb

justinsb

A few nits but otherwise LGTM

pkg/patterns/addon/pkg/status/basic.go

pkg/patterns/addon/pkg/status/version.go

justinsb · 2020-07-09T13:04:54Z

pkg/patterns/addon/pkg/status/version.go

+	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
+)
+
+// NewVersionCheck provides an implementation of declarative.Reconciled that


Oh cool - using a signature inspired by Reconcilers is an interesting pattern to explore!

justinsb · 2020-07-09T13:06:02Z

pkg/patterns/addon/pkg/status/version.go

+
+			versionActual, err := semver.Make(versionNeededStr)
+			if err != nil {
+				log.WithValues("version", versionNeededStr).Info("Unable to convert string to version, skipping this object")


Nit: I think you should report this as a log.Error(err), even if we don't return an error.

justinsb · 2020-07-09T13:08:09Z

pkg/patterns/addon/pkg/status/version.go

+		}
+	}
+
+	// TODO(somtochi): Do we want to return an error when the version is invalid or just skip and use the operator?


I actually think it would be reasonable to return an error, if the annotation is specified but we can't parse it. It could be a version schema our operator is too old to understand. (More likely, it's just a typo).

But ... the user did specify the annotation in their manifest...

I think returning an error is reasonable, it will help users create valid manifests. A message in the logs is going to get overlooked and users will be surprised when it doesn't work.

There are also two versions, what happens if we cannot parse the operator's version?

justinsb · 2020-07-09T13:10:03Z

pkg/patterns/addon/pkg/status/version.go

+	// TODO(somtochi): Do we want to return an error when the version is invalid or just skip and use the operator?
+	operatorVersion, err := semver.Make(p.version)
+	if err != nil {
+		log.WithValues("version", p.version).Info("Unable to convert string to version, skipping check")


You could move this to NewVersionCheck; I think it's very natural to return an error there if passed a bad version :-)

Related question is: should we refuse to apply manifests that specify invalid minimum versions (i.e. ignore vs error). I would vote "error" I think, based on my chain of thoughts in this review!

justinsb · 2020-07-09T13:11:03Z

pkg/patterns/declarative/reconciler.go

@@ -122,6 +122,22 @@ func (r *Reconciler) reconcileExists(ctx context.Context, name types.NamespacedN
 	}
 	log.WithValues("objects", fmt.Sprintf("%d", len(objects.Items))).Info("built deployment objects")

+	if r.options.status != nil {
+		bool, err := r.options.status.VersionCheck(ctx, instance, objects)


Nit: Don't name it bool - it's confusing. e.g. maybe something like validVersion, err := r.options.status.VersionCheck(...)

justinsb · 2020-07-09T13:11:26Z

pkg/patterns/declarative/reconciler.go

+		if  err != nil {
+			if !bool {
+				// r.client isn't exported so can't be updated in version check function
+				r.client.Status().Update(ctx, instance)


Nit: I think this can return an error; we need to bubble it up if so.

justinsb · 2020-07-27T12:38:49Z

pkg/patterns/addon/pkg/status/version.go

+	for _, obj := range objs.Items {
+		annotations := obj.UnstructuredObject().GetAnnotations()
+		if versionNeededStr, ok := annotations["addons.k8s.io/min-operator-version"]; ok {
+			log.WithValues("version", versionNeededStr).Info("Got version requirement addons.k8s.io/operator-version=%v")


Nit: this should say min-operator-version to match the annotation value

justinsb · 2020-07-27T12:40:51Z

pkg/patterns/declarative/reconciler.go

+		if err != nil {
+			if !isValidVersion {
+				// r.client isn't exported so can't be updated in version check function
+				err := r.client.Status().Update(ctx, instance)


So this actually "shadows" the err value we care about. So on line 142 err will be nil.

Sometimes people use a different name e.g. updateErr :=...

But I think a better pattern - when you can use it - is to so if err := r.client.Status()...; err != nil as then the scope of the shadowing err should just be the if statement (I think!)

justinsb · 2020-07-27T12:41:16Z

One problem with variable shadowing (and a nit), but otherwise lgtm

justinsb · 2020-08-10T13:24:18Z

Thanks @somtochiama ... this looks great

/approve
/lgtm
/ok-to-test

k8s-ci-robot · 2020-08-10T13:24:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, SomtochiAma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jul 3, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 3, 2020

k8s-ci-robot requested review from atoato88 and droot July 3, 2020 15:29

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 3, 2020

somtochiama changed the title ~~Update event status~~ Adds Preflight Check Jul 3, 2020

k8s-ci-robot assigned justinsb Jul 7, 2020

somtochiama changed the title ~~Adds Preflight Check~~ Adds Version Check Jul 7, 2020

justinsb reviewed Jul 9, 2020

View reviewed changes

somtochiama requested a review from justinsb July 10, 2020 07:06

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2020

justinsb reviewed Jul 27, 2020

View reviewed changes

somtochiama added 2 commits July 30, 2020 13:33

Base preflight check function

1b07d87

Base preflight check function

9e17933

somtochiama force-pushed the update-event-status branch from 49bb60f to 9e17933 Compare July 30, 2020 12:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2020

Resolves variable naming conflict

b8ba229

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2020

k8s-ci-robot merged commit a6536ef into kubernetes-sigs:master Aug 10, 2020

somtochiama mentioned this pull request Aug 14, 2020

REQUEST: New membership for SomtochiAma kubernetes/org#2107

Closed

6 tasks

somtochiama deleted the update-event-status branch August 20, 2020 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Version Check #102

Adds Version Check #102

somtochiama commented Jul 3, 2020

k8s-ci-robot commented Jul 3, 2020

somtochiama commented Jul 7, 2020

justinsb left a comment

justinsb Jul 9, 2020

justinsb Jul 9, 2020

justinsb Jul 9, 2020

somtochiama Jul 10, 2020

justinsb Jul 9, 2020

justinsb Jul 9, 2020

justinsb Jul 9, 2020

justinsb Jul 27, 2020

justinsb Jul 27, 2020

justinsb commented Jul 27, 2020

justinsb commented Aug 10, 2020

k8s-ci-robot commented Aug 10, 2020

Adds Version Check #102

Adds Version Check #102

Conversation

somtochiama commented Jul 3, 2020

k8s-ci-robot commented Jul 3, 2020

somtochiama commented Jul 7, 2020

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jul 27, 2020

justinsb commented Aug 10, 2020

k8s-ci-robot commented Aug 10, 2020