Read component version from ConfigMap #426

vinamra28 · 2021-09-24T16:32:53Z

Changes

Currently we were reading the component version from CRD but with
Pipelines version 0.25.x release and Triggers 0.15.x release the version
information is available in a ConfigMap and anyone who is
system:authenticated can read the version from ConfigMap. If the
configmap is not present then return Unknown as the version.

Closes #413

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

Fetch release version from ConfigMap for Pipelines and Triggers and if ConfigMap not available then return Unknown.

tekton-robot · 2021-09-24T16:36:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	78.3%	-0.3

tekton-robot · 2021-09-25T02:58:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	87.0%	8.4

pkg/reconciler/kubernetes/tektonpipeline/controller.go

tekton-robot · 2021-09-27T09:41:19Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	93.3%	14.8

sm43 · 2021-09-27T09:42:56Z

/lgtm

tekton-robot · 2021-09-27T09:45:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	93.3%	14.8

nikhil-thomas · 2021-09-27T10:09:21Z

pkg/reconciler/common/utils.go

+	if len(configMaps.Resources()) == 0 {
+		return "", fmt.Errorf("failed to find ConfigMap %s to get release version", configMapName)
 	}

-	crd := crds.Resources()[0]
-	version, ok := crd.GetLabels()[releaseLabel]
-	if !ok {
-		return version, fmt.Errorf("failed to find release label on crd")
+	versionConfigMap := configMaps.Resources()[0]
+	dataObj, _, _ := unstructured.NestedStringMap(versionConfigMap.Object, "data")
+	version := dataObj["version"]
+
+	if version != "" {
+		return version, nil
 	}

-	return version, nil
+	return "", fmt.Errorf("failed to find version data field on ConfigMap %s", configMapName)
 }


@vinamra28 when version is unknown (configmap not exist, or version field not exist) let us return unknown, instead of returning error.

Let us not return error and block/break installation for this reason. 🧑‍💻

@nikhil-thomas maybe we can return error from here and handle that in TektonPipeline and TektonTriggers controller. This way our error would be logged and we can set version to unknown over there. WDYT?

like ⬇️

var releaseVersion string = "unknown" // Read the release version of pipelines releaseVersion, err = common.FetchVersionFromConfigMap(manifest, versionConfigMap) if err != nil { logger.Infow("failed to read release version from ConfigMap", err) }

this could work,
but i would make it an internal implementation detail of releaseVersion(), because this error handling doesn't add more claritiy to the reconciler flow. 🧑‍💻

if you have already made this update, go ahead an push it. this should be fine 👍

why do we wanna show unknown? in what case that would happen?
nightly, release version, dev release yaml
all have version set with some value.
then if version is not set then doesn't it mean something is wrong?

then if version is not set then doesn't it mean something is wrong?

yes, but as this won't affect any critical functionality.
can we just do a warning. is the error and break necessary.

in addition, we will see the Unknown value in the CR status, i think that could be the necessary indicator that something is wrong 😇

this won't affect any critical functionality

if the release yaml is missing something then it can !!
i don't mind keeping it this way 😄 ... but if we assume version is missing i.e. something is wrong with release yaml and still we allow everything to install then we might break an existing working pipelines, triggers
if we stop then only operator would fail !

we can "type" the error and have a IsUnknow error check. From this part of the code perspective, it make sense to return an error. The consumer of this is the one with the responsability to handle it.

nikhil-thomas · 2021-09-27T10:11:05Z

/block

tekton-robot · 2021-09-27T14:55:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	93.3%	14.8

tekton-robot · 2021-09-27T14:56:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	93.3%	14.8

tekton-robot · 2021-09-27T15:10:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	93.3%	14.8

tekton-robot · 2021-10-01T08:10:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	87.5%	8.9

tekton-robot · 2021-10-01T08:34:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	87.5%	8.9

nikhil-thomas · 2021-10-05T11:43:17Z

/lgtm

nikhil-thomas · 2021-10-05T12:12:59Z

/hold

nikhil-thomas · 2021-10-05T12:14:27Z

adding a hold as @vinamra28 is making a minor change.

@vinamra28 please feel free to remove this hold at any point (with or without changes)

Currently we were reading the component version from CRD but with Pipelines version 0.25.x release and Triggers 0.15.x release the version information is available in a ConfigMap and anyone who is system:authenticated can read the version from ConfigMap. If the configmap is not present then return Unknown as the version.

tekton-robot · 2021-10-05T12:21:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	87.5%	8.9

tekton-robot · 2021-10-05T12:22:55Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	78.6%	87.5%	8.9

vinamra28 · 2021-10-05T12:23:48Z

adding a hold as @vinamra28 is making a minor change.

@vinamra28 please feel free to remove this hold at any point (with or without changes)

thanks @nikhil-thomas for this 👍
did the changes.
/hold cancel

nikhil-thomas · 2021-10-06T10:51:39Z

/lgtm

nikhil-thomas · 2021-10-06T10:52:04Z

/approve

tekton-robot · 2021-10-06T10:52:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhil-thomas

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nikhil-thomas]

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

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 24, 2021

tekton-robot requested review from piyush-garg and pradeepitm12 September 24, 2021 16:32

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 24, 2021

vinamra28 force-pushed the issues-413 branch from 1d53cb8 to 1c3a75f Compare September 25, 2021 02:55

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 25, 2021

sm43 reviewed Sep 27, 2021

View reviewed changes

pkg/reconciler/kubernetes/tektonpipeline/controller.go Outdated Show resolved Hide resolved

vinamra28 force-pushed the issues-413 branch from 1c3a75f to 0e3b49f Compare September 27, 2021 09:37

vinamra28 force-pushed the issues-413 branch from 0e3b49f to 056071e Compare September 27, 2021 09:41

tekton-robot assigned sm43 Sep 27, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021

nikhil-thomas reviewed Sep 27, 2021

View reviewed changes

vinamra28 force-pushed the issues-413 branch from 056071e to 2bd5f92 Compare September 27, 2021 14:51

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021

vinamra28 force-pushed the issues-413 branch from 2bd5f92 to 832371e Compare September 27, 2021 14:53

vinamra28 force-pushed the issues-413 branch from 832371e to 582eb82 Compare September 27, 2021 15:06

vinamra28 force-pushed the issues-413 branch from 582eb82 to fef48a5 Compare October 1, 2021 08:06

vinamra28 force-pushed the issues-413 branch from fef48a5 to b4cab7c Compare October 1, 2021 08:31

tekton-robot assigned nikhil-thomas Oct 5, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2021

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2021

vinamra28 force-pushed the issues-413 branch from b4cab7c to 7bb6b76 Compare October 5, 2021 12:16

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2021

vinamra28 force-pushed the issues-413 branch from 7bb6b76 to 6b688e4 Compare October 5, 2021 12:19

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2021

vinamra28 requested review from nikhil-thomas and sm43 October 6, 2021 05:48

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2021

tekton-robot merged commit f2113b6 into tektoncd:main Oct 6, 2021

Read component version from ConfigMap #426

Read component version from ConfigMap #426

Uh oh!

Conversation

vinamra28 commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Sep 24, 2021

Uh oh!

tekton-robot commented Sep 25, 2021

Uh oh!

Uh oh!

tekton-robot commented Sep 27, 2021

Uh oh!

sm43 commented Sep 27, 2021

Uh oh!

tekton-robot commented Sep 27, 2021

Uh oh!

nikhil-thomas Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

vinamra28 Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

vinamra28 Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

nikhil-thomas Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

nikhil-thomas Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

sm43 Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikhil-thomas Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikhil-thomas Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

sm43 Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

nikhil-thomas commented Sep 27, 2021

Uh oh!

tekton-robot commented Sep 27, 2021

Uh oh!

tekton-robot commented Sep 27, 2021

Uh oh!

tekton-robot commented Sep 27, 2021

Uh oh!

tekton-robot commented Oct 1, 2021

Uh oh!

tekton-robot commented Oct 1, 2021

Uh oh!

nikhil-thomas commented Oct 5, 2021

Uh oh!

nikhil-thomas commented Oct 5, 2021

Uh oh!

nikhil-thomas commented Oct 5, 2021

Uh oh!

tekton-robot commented Oct 5, 2021

Uh oh!

tekton-robot commented Oct 5, 2021

Uh oh!

vinamra28 commented Oct 5, 2021

Uh oh!

nikhil-thomas commented Oct 6, 2021

Uh oh!

vinamra28 commented Sep 24, 2021 •

edited

Loading

sm43 Sep 27, 2021 •

edited

Loading

nikhil-thomas Sep 27, 2021 •

edited

Loading

sm43 Sep 28, 2021 •

edited

Loading