Add Dev and Project labeling / refactoring labeling and annotations #5551

cdrage · 2022-03-14T13:48:36Z

What type of PR is this:

/kind feature
/kind cleanup

What does this PR do / why we need it:

In this PR we:

Correctly name all the labels in a more concise manner (ex. Project Type
is an annotation)
Add Dev and Deploy mode labels to both odo dev and odo deploy
commands so that we can intuitively retrieve what components are in
deploy or dev mode
Somewhat refactors how we do labeling

Which issue(s) this PR fixes:

1/2 of PR's for #5405

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

N/A

Signed-off-by: Charlie Drage charlie@charliedrage.com

netlify · 2022-03-14T13:48:40Z

✔️ Deploy Preview for odo-docusaurus-preview canceled.

🔨 Explore the source changes: 6e42019

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6231e96af8aa1f0007ed8fbe

openshift-ci · 2022-03-14T13:48:42Z

@cdrage: The label(s) kind/cleanup cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this:

/kind feature
/kind cleanup

What does this PR do / why we need it:

In this PR we:

Correctly name all the labels in a more concise manner (ex. Project Type
is an annotation)

Add Dev and Deploy mode labels to both odo dev and odo deploy
commands so that we can intuitively retrieve what components are in
deploy or dev mode

Somewhat refactors how we do labeling

Which issue(s) this PR fixes:

1/2 of PR's for #5405

PR acceptance criteria:

Unit test

Integration test

Documentation

How to test changes / Special notes to the reviewer:

N/A

Signed-off-by: Charlie Drage charlie@charliedrage.com

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.

odo-robot · 2022-03-14T13:55:55Z

Unit Tests on commit 4ec08b6 finished successfully.
View logs: TXT HTML

odo-robot · 2022-03-14T14:05:23Z

Validate Tests on commit 4ec08b6 finished successfully.
View logs: TXT HTML

**What type of PR is this:**  /kind feature /kind cleanup **What does this PR do / why we need it:** In this PR we: * Correctly name all the labels in a more concise manner (ex. Project Type is an annotation) * Add Dev and Deploy mode labels to both `odo dev` and `odo deploy` commands so that we can intuitively retrieve what components are in deploy or dev mode * Somewhat refactors how we do labeling **Which issue(s) this PR fixes:**  1/2 of PR's for redhat-developer#5405 **PR acceptance criteria:** - [X] Unit test - [X] Integration test - [X] Documentation **How to test changes / Special notes to the reviewer:** N/A Signed-off-by: Charlie Drage <charlie@charliedrage.com>

odo-robot · 2022-03-14T14:18:46Z

Kubernetes Tests on commit 4ec08b6 finished successfully.
View logs: TXT HTML

odo-robot · 2022-03-14T14:23:19Z

Windows Tests (OCP) on commit 4ec08b6 finished with errors.
View logs: TXT HTML

odo-robot · 2022-03-14T14:30:23Z

OpenShift Tests on commit 4ec08b6 finished with errors.
View logs: TXT HTML

feloy

I note that this PR does not fix #5546, that's ok for me (I prefer that it is fixed as part of another PR)

pkg/devfile/adapters/kubernetes/component/adapter.go

pkg/component/labels/labels.go

valaparthvi · 2022-03-15T12:29:40Z

@cdrage Can you try to get this PR in as soon as possible? I think it will be helpful for the odo delete PR that I am working on.

feloy · 2022-03-15T13:15:20Z

You will need to run go fmt ./... to format files correctly:

gofmt ERROR - These files are not formated by gofmt:
./pkg/component/component_test.go
./pkg/component/labels/labels_test.go
./pkg/devfile/adapters/kubernetes/component/adapter_test.go
./pkg/kclient/fake/ingress.go
./pkg/storage/labels/labels_test.go
./pkg/testingutil/deployments.go
./pkg/testingutil/routes.go
./pkg/url/labels/labels_test.go

kadel · 2022-03-15T13:25:28Z

pkg/component/labels/labels.go

-// ComponentDeployLabel ...
-const ComponentDeployLabel = "Deploy"
+// ComponentUnknownLabel is the label that is used to display something we do not know
+const ComponentUnknownLabel = "<unknown>"


This is not used anywhere.

kadel · 2022-03-15T13:26:04Z

pkg/component/labels/labels.go

+const ComponentDeployName = "Deploy"
+
+// ComponentNoneName ...
+const ComponentNoneName = "None"


This is not used anywhere.

kadel · 2022-03-15T13:27:25Z

pkg/component/component.go

@@ -413,13 +415,14 @@ func getRemoteComponentMetadata(client kclient.ClientInterface, componentName st
 	// Annotations
 	component.Annotations = fromCluster.GetAnnotations()

+	// Mark the component status as pushed
+	component.Status.State = componentlabels.ComponentPushedName


This should probably use StateTypePushed from types.go

kadel · 2022-03-15T13:28:24Z

pkg/component/labels/labels.go

+const ComponentNoneName = "None"
+
+// ComponentPushedName ...
+const ComponentPushedName = "Pushed"


This is not used as a label or its value it should not be in this file.

Similar constant is already defined in pkg/component/types.go

kadel · 2022-03-15T13:29:27Z

pkg/component/component.go

 	"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
 	"github.com/devfile/api/v2/pkg/devfile"
 	"github.com/devfile/library/pkg/devfile/parser"
 	parsercommon "github.com/devfile/library/pkg/devfile/parser/data/v2/common"
 	dfutil "github.com/devfile/library/pkg/util"

-	applabels "github.com/redhat-developer/odo/pkg/application/labels"


can we keep the import grouping as it were?

Signed-off-by: Charlie Drage <charlie@charliedrage.com>

pkg/component/component.go

pkg/component/pushed_component.go

kadel · 2022-03-15T13:39:19Z

pkg/devfile/adapters/kubernetes/component/adapter.go

+	// Since `odo deploy` can theoretically deploy a deployment as well with the same instance name
+	// we make sure that we are retrieving the deployment with the Dev mode, NOT Deploy.
+	selectorLabels := componentlabels.GetLabels(a.ComponentName, a.AppName, false)
+	selectorLabels[componentlabels.OdoModeLabel] = componentlabels.ComponentDevName


wouldn't it be better to include OdoModeLabel in componentlabels.GetLabels?
It will be safer and it it will be automatically used everywhere where it is needed.

I don't think so unless there is a refactor with the GetLabels and how we do it elsewhere (it'd break odo push tests).

I did think of your solution, but found I was changing WAYYY too many places where the GetLabels code is. Especially in parts of the code where you would have to modify multiple functions just to pass in "Dev" or "Deploy" from the pkg/cli layer.

pkg/devfile/adapters/kubernetes/component/adapter.go

pkg/devfile/adapters/kubernetes/component/adapter_test.go

valaparthvi · 2022-03-15T15:01:11Z

@cdrage I'm no longer blocked on this, so take your time.

cdrage · 2022-03-16T13:26:58Z

/retest

…labels Signed-off-by: Charlie Drage <charlie@charliedrage.com>

sonarcloud · 2022-03-16T13:45:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
1.0% Duplication

feloy · 2022-03-16T16:42:50Z

/retest

feloy · 2022-03-16T16:43:09Z

/retest

 failed to pull stack java-openliberty from registry.stage.devfile.io/devfile-catalog/java-openliberty:0.8.1 with allowed media types [application/vnd.devfileio.devfile.layer.v1 image/png image/svg+xml application/vnd.devfileio.vsx.layer.v1.tar application/x-tar]: failed to copy: httpReaderSeeker: failed open: unexpected status code https://registry.stage.devfile.io/v2/devfile-catalog/java-openliberty/blobs/sha256:99c7dfff0973a1f1a036ad5c7c8c0d87d05f97d161ac9d49881d6fc152f5baf0: 502 Bad Gateway

feloy · 2022-03-16T16:47:50Z

/test v4.9-integration-e2e

feloy · 2022-03-17T08:49:09Z

/lgtm

dharmit · 2022-03-17T10:17:59Z

pkg/devfile/adapters/kubernetes/component/adapter.go

@@ -254,7 +266,7 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {

 	a.deployment, err = a.Client.WaitForDeploymentRollout(a.deployment.Name)
 	if err != nil {
-		return errors.Wrap(err, "error while waiting for deployment rollout")
+		return errors.Wrapf(err, "Failed to update config to component deployed.")


There's a separate PR for removing the occurrences of errors.Wrapf, but wherever you are modifying things, can you make sure to use fmt.Errorf? Otherwise, I think, we will end up having errors.Wrapf occurrences if that PR goes in before yours.

@feloy WDYT? I see you already lgtm'd this PR. Since you are working on the other PR, I am OK by adding LGTM back to this PR if you would prefer this one to go in first.

We will turn @cdrage into a weather vane (is this phrase existing in english?), as he already changed this back and forth several times depending on different reviews :(
I created the PR #5557 to resolve the misunderstanding, let's keep it as is here, I will handle it in my PR after rebasing

feloy · 2022-03-17T12:18:57Z

/approve

openshift-ci · 2022-03-17T12:24:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

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~~ [feloy]

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

…edhat-developer#5551) * Add Dev and Project labeling / refactoring labeling and annotations  **What type of PR is this:**  /kind feature /kind cleanup **What does this PR do / why we need it:** In this PR we: * Correctly name all the labels in a more concise manner (ex. Project Type is an annotation) * Add Dev and Deploy mode labels to both `odo dev` and `odo deploy` commands so that we can intuitively retrieve what components are in deploy or dev mode * Somewhat refactors how we do labeling **Which issue(s) this PR fixes:**  1/2 of PR's for redhat-developer#5405 **PR acceptance criteria:** - [X] Unit test - [X] Integration test - [X] Documentation **How to test changes / Special notes to the reviewer:** N/A Signed-off-by: Charlie Drage <charlie@charliedrage.com> * Review update / renaming Signed-off-by: Charlie Drage <charlie@charliedrage.com> * Review changes. Adds tests for generateDeploymentObjectMeta. Changes labels Signed-off-by: Charlie Drage <charlie@charliedrage.com>

openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 14, 2022

openshift-ci bot requested review from anandrkskd and dharmit March 14, 2022 13:48

cdrage force-pushed the refactor-labels branch from a01483f to 2d8a402 Compare March 14, 2022 13:55

cdrage force-pushed the refactor-labels branch from 2d8a402 to b6aa986 Compare March 14, 2022 14:10

This was referenced Mar 14, 2022

Implement redesigned odo list #5468

Closed

Implement redesigned odo list functionality #5552

Merged

feloy suggested changes Mar 14, 2022

View reviewed changes

pkg/devfile/adapters/kubernetes/component/adapter.go Show resolved Hide resolved

pkg/component/labels/labels.go Show resolved Hide resolved

pkg/component/labels/labels.go Outdated Show resolved Hide resolved

openshift-ci bot assigned feloy Mar 14, 2022