Use map for runningIn instead of array in JSON output #6094

feloy · 2022-09-05T11:33:58Z

What type of PR is this:

/kind bug

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #5999

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

netlify · 2022-09-05T11:34:09Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`820eddc`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/63175bd3f5b17c00088555da

odo-robot · 2022-09-05T11:42:11Z

Unit Tests on commit 508a8e4 finished successfully.
View logs: TXT HTML

odo-robot · 2022-09-05T11:49:27Z

Validate Tests on commit 508a8e4 finished successfully.
View logs: TXT HTML

odo-robot · 2022-09-05T11:57:19Z

OpenShift Tests on commit 508a8e4 finished successfully.
View logs: TXT HTML

odo-robot · 2022-09-05T11:58:48Z

Kubernetes Tests on commit 508a8e4 finished successfully.
View logs: TXT HTML

valaparthvi · 2022-09-06T09:02:57Z

tests/integration/cmd_devfile_list_test.go

-				helper.JsonPathContentContain(stdout, "components.0.runningIn.#", "1")
-				helper.JsonPathContentContain(stdout, "components.0.runningIn.0", "Dev")
+				helper.JsonPathContentContain(stdout, "components.0.runningIn.dev", "true")
+				helper.JsonPathContentContain(stdout, "components.0.runningIn.deploy", "")


nit: I find this check confusing. If the component is not running in Deploy mode, "deploy" will not appear in the list, why then check for it to be an empty string.

The JsonPathContentContain function is not able to make a difference between is the field is not defined or if the field is empty.

This test is used to check that the field is not defined

feloy · 2022-09-06T09:18:27Z

Re-run with new tests names

rm3l · 2022-09-06T08:12:57Z

pkg/api/component.go

 	"strings"
 )

 type RunningMode string
-type RunningModeList []RunningMode
+type RunningModeList map[string]bool

 const (
 	RunningModeDev     RunningMode = "Dev"


I can see that those constants (and the RunningMode type) are no longer used and could be removed.

rm3l · 2022-09-06T08:14:58Z

pkg/api/component.go

-		strs = append(strs, string(s))
+	for s, v := range o {
+		if v {
+			strs = append(strs, string(strings.Title(s)))


strings.Title already returns a string, so the additional type conversion does not seem needed.

rm3l · 2022-09-06T09:28:41Z

docs/website/docs/command-reference/json-output.md

@@ -157,7 +157,9 @@ odo describe component -o json
 			"containerPort": 3000
 		}
 	],
-	"runningIn": ["Dev"],
+	"runningIn": {
+		"dev": true


Feature-wise, I was expecting the list of possible running modes to be exposed, instead of just the current running mode.

From what I understand from this comment:

menu to start dev mode is enabled if devfile has dev support and if dev mode is not running for the component.

If devfile has dev support but dev mode is not running for the component, I think odo describe component -o json should have "dev": false reported in the running mode object, to help to parse that information. No?

Yes, you are right. Writing the doc, I was thinking it was due to the omitempty nature of the marshalling in Go, but it is not the case, because it is a map. I'll make the changes to add by default all the keys with a False value

odo-robot · 2022-09-06T14:02:16Z

Windows Tests (OCP) on commit 508a8e4 finished successfully.
View logs: TXT HTML

pkg/api/component.go

feloy · 2022-09-06T14:40:34Z

Thanks @dharmit for your review

sonarcloud · 2022-09-06T14:41:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

rm3l

Thanks for the changes!

/approve

openshift-ci · 2022-09-06T15:05:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l

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

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

dharmit · 2022-09-06T15:57:55Z

/lgtm

feloy · 2022-09-06T16:22:56Z

/override ci/prow/v4.10-integration-e2e
Tests pass on IBM cloud

openshift-ci · 2022-09-06T16:23:04Z

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e
Tests pass on IBM cloud

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.

feloy · 2022-09-06T16:41:17Z

/override ci/prow/v4.10-integration-e2e
/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
/override OpenShift-Integration-tests/OpenShift-Integration-tests
/override Unit-Tests/Unit-Tests

All tests passed, but not recognized by Github

Required status checks "Kubernetes-Integration-Tests/Kubernetes-Integration-Tests", "OpenShift-Integration-tests/OpenShift-Integration-tests", and "Unit-Tests/Unit-Tests" were not set by the expected GitHub apps.

openshift-ci · 2022-09-06T16:43:14Z

@feloy: Overrode contexts on behalf of feloy: Kubernetes-Integration-Tests/Kubernetes-Integration-Tests, OpenShift-Integration-tests/OpenShift-Integration-tests, Unit-Tests/Unit-Tests, ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e
/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
/override OpenShift-Integration-tests/OpenShift-Integration-tests
/override Unit-Tests/Unit-Tests
Required status checks "Kubernetes-Integration-Tests/Kubernetes-Integration-Tests", "OpenShift-Integration-tests/OpenShift-Integration-tests", and "Unit-Tests/Unit-Tests" were not set by the expected GitHub apps.

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.

feloy requested a review from valaparthvi September 5, 2022 11:34

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 5, 2022

feloy requested a review from rm3l September 5, 2022 11:34

openshift-ci bot requested a review from anandrkskd September 5, 2022 11:34

feloy removed the request for review from anandrkskd September 5, 2022 11:34

feloy force-pushed the bugfix-5999/odo-list-json branch from 2755cb7 to 9d72e39 Compare September 5, 2022 11:43

valaparthvi approved these changes Sep 6, 2022

View reviewed changes

openshift-ci bot assigned valaparthvi Sep 6, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 6, 2022

feloy closed this Sep 6, 2022

feloy reopened this Sep 6, 2022

rm3l requested changes Sep 6, 2022

View reviewed changes

openshift-ci bot assigned rm3l Sep 6, 2022

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 6, 2022

feloy force-pushed the bugfix-5999/odo-list-json branch from 9d72e39 to c43d99c Compare September 6, 2022 12:02

feloy added 2 commits September 6, 2022 15:30

Use map for runningIn instead of array

10b66ab

Set false entries in runningIn map

289c742

feloy force-pushed the bugfix-5999/odo-list-json branch from c43d99c to 289c742 Compare September 6, 2022 13:31

feloy requested review from rm3l and valaparthvi September 6, 2022 13:58

dharmit suggested changes Sep 6, 2022

View reviewed changes

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

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

openshift-ci bot assigned dharmit Sep 6, 2022

Rename RunningModeList to RunningModes + fix doc

6dd42d7

Fix integration test

820eddc

feloy requested a review from dharmit September 6, 2022 14:40

rm3l reviewed Sep 6, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 6, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 6, 2022

feloy merged commit 9e39df3 into redhat-developer:main Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use map for runningIn instead of array in JSON output #6094

Use map for runningIn instead of array in JSON output #6094

feloy commented Sep 5, 2022

netlify bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

valaparthvi Sep 6, 2022

feloy Sep 6, 2022

feloy commented Sep 6, 2022

rm3l Sep 6, 2022

rm3l Sep 6, 2022

rm3l Sep 6, 2022

feloy Sep 6, 2022

odo-robot bot commented Sep 6, 2022 •

edited

Loading

feloy commented Sep 6, 2022

sonarcloud bot commented Sep 6, 2022

rm3l left a comment

openshift-ci bot commented Sep 6, 2022

dharmit commented Sep 6, 2022

feloy commented Sep 6, 2022

openshift-ci bot commented Sep 6, 2022

feloy commented Sep 6, 2022 •

edited

Loading

openshift-ci bot commented Sep 6, 2022

Use map for runningIn instead of array in JSON output #6094

Use map for runningIn instead of array in JSON output #6094

Conversation

feloy commented Sep 5, 2022

netlify bot commented Sep 5, 2022 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

odo-robot bot commented Sep 5, 2022 • edited Loading

odo-robot bot commented Sep 5, 2022 • edited Loading

odo-robot bot commented Sep 5, 2022 • edited Loading

odo-robot bot commented Sep 5, 2022 • edited Loading

valaparthvi Sep 6, 2022

Choose a reason for hiding this comment

feloy Sep 6, 2022

Choose a reason for hiding this comment

feloy commented Sep 6, 2022

rm3l Sep 6, 2022

Choose a reason for hiding this comment

rm3l Sep 6, 2022

Choose a reason for hiding this comment

rm3l Sep 6, 2022

Choose a reason for hiding this comment

feloy Sep 6, 2022

Choose a reason for hiding this comment

odo-robot bot commented Sep 6, 2022 • edited Loading

feloy commented Sep 6, 2022

sonarcloud bot commented Sep 6, 2022

rm3l left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 6, 2022

dharmit commented Sep 6, 2022

feloy commented Sep 6, 2022

openshift-ci bot commented Sep 6, 2022

feloy commented Sep 6, 2022 • edited Loading

openshift-ci bot commented Sep 6, 2022

netlify bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 5, 2022 •

edited

Loading

odo-robot bot commented Sep 6, 2022 •

edited

Loading

feloy commented Sep 6, 2022 •

edited

Loading