Personalize devfile #5467

valaparthvi · 2022-02-17T16:52:35Z

What type of PR is this:
/kind feature

What does this PR do / why we need it:
This PR allows user to personalize their devfile by adding/removing port, and environment variable.
Which issue(s) this PR fixes:

Fixes #5423

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

netlify · 2022-02-17T16:52:41Z

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

🔨 Explore the source changes: ff7726c

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

odo-robot · 2022-02-17T16:57:29Z

Validate Tests on commit 58a3b27 finished successfully.
View logs: TXT HTML

odo-robot · 2022-02-17T16:59:29Z

Unit Tests on commit 58a3b27 finished successfully.
View logs: TXT HTML

odo-robot · 2022-02-17T17:04:55Z

OpenShift Tests on commit 58a3b27 finished successfully.
View logs: TXT HTML

odo-robot · 2022-02-17T17:51:44Z

Kubernetes Tests on commit 58a3b27 finished with errors.
View logs: TXT HTML

odo-robot · 2022-02-17T21:39:37Z

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

feloy

I really like how this code is articulated 🚀
Some remarks around documenting and testing it

feloy · 2022-02-22T15:06:03Z

pkg/init/asker/asker.go

+	return selectContainerAnswer, nil
+}
+
+type ContainerConfiguration struct {


It would be helpful to have these types definitions into the asker/interface.go file, as they are part of the interface definition, and to document them, so the documentation reader can understand how data is exchanged with the methods.

feloy · 2022-02-22T15:12:03Z

pkg/init/backend/interactive.go

+				return err
+			}
+
+			if configOps.Ops == "Delete" && configOps.Kind == "Port" {


Could we do this switch with switch/case instead of if/else, so the reader is sure there is no other parameters than Ops and Kind

I am not sure if f3c7cf4 is what you meant, let me know if not.

this syntax is pretty similar to the if/else, as the content of the case is free.

I was thinking of this form, which reduces the possibilities in the cases (pseudo-code):

switch Ops { case "Delete": switch Kind: case "Port": [...] case "EnvVar": [...]

feloy · 2022-02-22T15:16:28Z

pkg/init/asker/asker.go

+	Envs  map[string]string
+}
+
+type ContainerMap struct {


This name is not speaking to me, could it be something similar to OperationOnContainer instead?

feloy · 2022-02-22T15:19:32Z

pkg/init/asker/asker.go

+
+// AskPersonalizeConfiguration asks the configuration user wants to change
+func (o *Survey) AskPersonalizeConfiguration(configuration ContainerConfiguration) (ContainerMap, error) {
+	options := []string{


If you want to run unit test on building the options/tracker, it would be helpful to extract the first part of this method to a specific method that builds options/tracker based on configuration, so you can test the options/tracker content based on different ports/envs.

So, writing a method proved to be a pain for me while writing unit test for PersonalizeDevfileconfig method of interactive.go. Since it's purely logical/mathematical, I decided to write a function, and testing shouldn't be difficult.

Signed-off-by: Parthvi Vala <[email protected]>

valaparthvi · 2022-02-23T12:15:09Z

I really like how this code is articulated rocket

😄 I can't take the whole credit, Tomas's prototype did most work.

@feloy I am having a hard time getting env var related unit tests to pass, can you help me with those?

feloy · 2022-02-23T12:37:46Z

I really like how this code is articulated rocket

smile I can't take the whole credit, Tomas's prototype did most work.

@feloy I am having a hard time getting env var related unit tests to pass, can you help me with those?

You seem pretty close. The function gomock.Any() will help if you don't need to assert what is passed to the method you are mocking:

For example:

client.EXPECT().AskPersonalizeConfiguration(gomock.Any())

valaparthvi · 2022-02-23T13:05:15Z

I really like how this code is articulated rocket

smile I can't take the whole credit, Tomas's prototype did most work.
@feloy I am having a hard time getting env var related unit tests to pass, can you help me with those?

You seem pretty close. The function gomock.Any() will help if you don't need to assert what is passed to the method you are mocking:

For example:
client.EXPECT().AskPersonalizeConfiguration(gomock.Any())

I would prefer explicit args and calls because we return different result for different set of args. What I don;t understand is why it works for Port, but not Env Var!

Edit: But you're right, using gomock.Any() second time fixed the failing tests. I have my doubts about it being a false positive, but then checkResults work as expected, so I will go with it for now.

Signed-off-by: Parthvi Vala <[email protected]>

…is interactive mode

Signed-off-by: Parthvi Vala <[email protected]>

feloy · 2022-02-23T15:15:11Z

pkg/init/backend/interactive.go

+func PrintConfiguration(config asker.DevfileConfiguration) {
+	color.New(color.Bold, color.FgGreen).Println("Current component configuration:")
+
+	for key, container := range config {


config being a map, the elements will be rendered in a random order every time. I think it could be more readable by sorting the keys first

feloy · 2022-02-23T15:22:01Z

pkg/init/backend/interactive.go

@@ -104,3 +106,149 @@ func (o *InteractiveBackend) PersonalizeName(devfile parser.DevfileObj, flags ma
 	}
 	return devfile.SetMetadataName(name)
 }
+
+func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileObj) error {


As discussed with Tomas on Slack, could you pass the devfile data only here, and execute devfileobj.WriteYamlDevfile() at the CLI level, so we remove the Filesystem dependency for this method?

I feel it makes more sense to keep things as is. There is PersonalizeName as well that has filesystem dependency, to be consistent with it, I think current code should work just fine.

feloy · 2022-02-23T18:04:48Z

pkg/init/backend/interactive.go

@@ -211,12 +212,18 @@ func (o *InteractiveBackend) PersonalizeDevfileconfig(devfileobj parser.DevfileO
 func PrintConfiguration(config asker.DevfileConfiguration) {
 	color.New(color.Bold, color.FgGreen).Println("Current component configuration:")

-	for key, container := range config {
+	keys := make([]string, len(config))


Doing this, you get a slice with n elemens already on it, and later with append you add new ones: you will finish with 2*n elements.
Either you keep this make and use keys[i] = key, or you can use make([]string, 0, len(config)) to use append.

Signed-off-by: Parthvi Vala <[email protected]>

sonarqubecloud · 2022-02-24T05:46:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.5% Duplication

valaparthvi · 2022-02-24T07:17:31Z

/retest

�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m�[32m•�[0m
�[90m------------------------------�[0m
�[91m�[1m• Failure [45.731 seconds]�[0m
odo devfile push command tests
�[90m/go/src/github.com/redhat-developer/odo/tests/integration/devfile/cmd_devfile_push_test.go:18�[0m
  when Create and push java-springboot component
  �[90m/go/src/github.com/redhat-developer/odo/tests/integration/devfile/cmd_devfile_push_test.go:791�[0m
    �[91m�[1mshould execute default build and run commands correctly [It]�[0m
    �[90m/go/src/github.com/redhat-developer/odo/tests/integration/devfile/cmd_devfile_push_test.go:800�[0m

    �[91mUnexpected error:
        <*errors.errorString | 0xc00061c510>: {
            s: "cmd [ps -ef] failed with error Signal 23 (URG) caught by ps (3.3.15).\nps:ps/display.c:66: please report this bug\ncommand terminated with exit code 1\n on pod loyezq-app-5cc8599984-glff7",
        }
        cmd [ps -ef] failed with error Signal 23 (URG) caught by ps (3.3.15).
        ps:ps/display.c:66: please report this bug
        command terminated with exit code 1
         on pod loyezq-app-5cc8599984-glff7
    occurred�[0m

    /go/src/github.com/redhat-developer/odo/tests/integration/devfile/cmd_devfile_push_test.go:824

feloy · 2022-02-24T07:20:20Z

Thanks for this work @valaparthvi
/lgtm
/approve

openshift-ci · 2022-02-24T07:20:29Z

[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

* Fix: Ask for devfile personalization * Update devfile library Signed-off-by: Parthvi Vala <[email protected]> * Add todo * Print live configuration, make it work * Run mockgen script Signed-off-by: Parthvi Vala <[email protected]> * TODO * Add missing mock methods * Add review suggestions * Add mock methods * Add unit tests * Add mock methods post rebase * Fixes post rebase Signed-off-by: Parthvi Vala <[email protected]> * Review, only run devfile personalization if the dir is not empty and is interactive mode * Mock methods * Fix unit tests Signed-off-by: Parthvi Vala <[email protected]> * Review Signed-off-by: Parthvi Vala <[email protected]>

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

openshift-ci bot requested review from feloy and mohammedzee1000 February 17, 2022 16:52

valaparthvi marked this pull request as draft February 17, 2022 16:52

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Feb 17, 2022

valaparthvi mentioned this pull request Feb 17, 2022

Draft: Ask for Devfile Personalization #5458

Closed

3 tasks

valaparthvi marked this pull request as ready for review February 22, 2022 12:03

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Feb 22, 2022

openshift-ci bot requested review from cdrage and dharmit February 22, 2022 12:03

feloy reviewed Feb 22, 2022

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Feb 23, 2022

valaparthvi added 11 commits February 23, 2022 17:28

Fix: Ask for devfile personalization

fdc3405

Update devfile library

1b04b1d

Signed-off-by: Parthvi Vala <[email protected]>

Add todo

8d4c37a

Print live configuration, make it work

3e2ebce

Run mockgen script

f221f2d

Signed-off-by: Parthvi Vala <[email protected]>

TODO

db45db5

Add missing mock methods

de1d39b

Add review suggestions

f3c7cf4

Add mock methods

f1f0e7d

Add unit tests

2dea56b

Add mock methods post rebase

572ffaa

valaparthvi force-pushed the personalizeDevfile branch from 0e8e1f5 to eaab1fe Compare February 23, 2022 12:08

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Feb 23, 2022

valaparthvi force-pushed the personalizeDevfile branch from eaab1fe to 963904c Compare February 23, 2022 12:31

Fixes post rebase

823d5b5

Signed-off-by: Parthvi Vala <[email protected]>

valaparthvi force-pushed the personalizeDevfile branch from 963904c to 823d5b5 Compare February 23, 2022 13:12

valaparthvi added 3 commits February 23, 2022 19:39

Review, only run devfile personalization if the dir is not empty and …

7bb78c4

…is interactive mode

Mock methods

262ff07

Fix unit tests

8410eae

Signed-off-by: Parthvi Vala <[email protected]>

valaparthvi force-pushed the personalizeDevfile branch from 2a790fa to 8410eae Compare February 23, 2022 14:44

valaparthvi requested a review from feloy February 23, 2022 14:51

openshift-ci bot assigned feloy Feb 23, 2022

feloy suggested changes Feb 23, 2022

View reviewed changes

Review

ff7726c

Signed-off-by: Parthvi Vala <[email protected]>

valaparthvi force-pushed the personalizeDevfile branch from ad3f8ad to ff7726c Compare February 24, 2022 05:45

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

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

openshift-merge-robot merged commit 39c8e40 into redhat-developer:main Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Personalize devfile #5467

Personalize devfile #5467

valaparthvi commented Feb 17, 2022

netlify bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

feloy left a comment

feloy Feb 22, 2022

feloy Feb 22, 2022

valaparthvi Feb 23, 2022

feloy Feb 23, 2022

feloy Feb 22, 2022 •

edited

Loading

feloy Feb 22, 2022

valaparthvi Feb 23, 2022

valaparthvi commented Feb 23, 2022 •

edited

Loading

feloy commented Feb 23, 2022

valaparthvi commented Feb 23, 2022 •

edited

Loading

feloy Feb 23, 2022

feloy Feb 23, 2022

valaparthvi Feb 23, 2022

feloy Feb 23, 2022

sonarqubecloud bot commented Feb 24, 2022

valaparthvi commented Feb 24, 2022

feloy commented Feb 24, 2022

openshift-ci bot commented Feb 24, 2022

Personalize devfile #5467

Personalize devfile #5467

Conversation

valaparthvi commented Feb 17, 2022

netlify bot commented Feb 17, 2022 • edited Loading

odo-robot bot commented Feb 17, 2022 • edited Loading

odo-robot bot commented Feb 17, 2022 • edited Loading

odo-robot bot commented Feb 17, 2022 • edited Loading

odo-robot bot commented Feb 17, 2022 • edited Loading

odo-robot bot commented Feb 17, 2022 • edited Loading

feloy 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

feloy Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi commented Feb 23, 2022 • edited Loading

feloy commented Feb 23, 2022

valaparthvi commented Feb 23, 2022 • edited Loading

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

sonarqubecloud bot commented Feb 24, 2022

valaparthvi commented Feb 24, 2022

feloy commented Feb 24, 2022

openshift-ci bot commented Feb 24, 2022

netlify bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

odo-robot bot commented Feb 17, 2022 •

edited

Loading

feloy Feb 22, 2022 •

edited

Loading

valaparthvi commented Feb 23, 2022 •

edited

Loading

valaparthvi commented Feb 23, 2022 •

edited

Loading