Add and remove service info from devfile #4465

dharmit · 2021-03-01T09:26:27Z

What type of PR is this?
/kind feature
/kind code-refactoring

What does this PR do / why we need it:

Implements the feature to add CRD and ServiceInstance information into devfile.yaml
Refactors odo service create and odo service delete CLI packages to use interface

Which issue(s) this PR fixes:

Addresses acceptance criteria discussed in:

It also makes changes to pkg/envinfo/storage.go to be compatible with changes introduced in devfile/library.

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

odo service create should add CRD and ServiceInstance information to devfile.yaml.
odo service delete should remove the CRD information from devfile.yaml.
odo storage create and odo storage delete should work as they work in current master branch. Code for these commands was changed to be compatible with changes introduced in devfile/library.

openshift-ci-robot · 2021-03-01T09:26:38Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2021-03-01T09:26:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

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

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

dharmit · 2021-03-15T09:28:37Z

Unit test failed locally with following error:

?   	github.com/openshift/odo/cmd/cli-doc	[no test files]
?   	github.com/openshift/odo/cmd/odo	[no test files]
ok  	github.com/openshift/odo/pkg/application	(cached)
ok  	github.com/openshift/odo/pkg/application/labels	(cached)
?   	github.com/openshift/odo/pkg/auth	[no test files]
ok  	github.com/openshift/odo/pkg/catalog	(cached)
ok  	github.com/openshift/odo/pkg/component	(cached)
ok  	github.com/openshift/odo/pkg/component/labels	(cached)
ok  	github.com/openshift/odo/pkg/config	(cached)
ok  	github.com/openshift/odo/pkg/debug	(cached)
ok  	github.com/openshift/odo/pkg/devfile/adapters	(cached)
 ⚠  there should be exactly one default command for command group build, currently there is no default command
 ⚠  there should be exactly one default command for command group debug, currently there is no default command
--- FAIL: TestGetCommandFromFlag (0.00s)
    --- FAIL: TestGetCommandFromFlag/Case_9:_Valid_devfile_with_invalid_composite_commands (0.00s)
        command_test.go:803: TestGetCommand unexpected error for command: build wantErr: true err: <nil>
 •  Executing command1 command ""  ...
 ✓  Executing command1 command "" [1ms]
 •  Executing command2 command ""  ...
 ✓  Executing command2 command "" [3ms]
 •  Executing command1 command ""  ...
 ✗  Executing command1 command "" [734461ns]
 •  Executing command1 command ""  ...
 •  Executing command2 command ""  ...
 ✓  Executing command1 command "" [6ms]
 ✓  Executing command2 command "" [6ms]
 •  Executing command2 command ""  ...
 •  Executing command1 command ""  ...
 ✗  Executing command2 command "" [1ms]
 ✗  Executing command1 command "" [1ms]
 •  Executing command1 command ""  ...
 ✓  Executing command1 command "" [448100ns]
 •  Executing command1 command ""  ...
 ✓  Executing command1 command "" [478143ns]
 •  Executing command2 command ""  ...
 ✓  Executing command2 command "" [519094ns]
 •  Executing command1 command ""  ...
 •  Executing command1 command ""  ...
 ✓  Executing command1 command "" [5ms]
 ✓  Executing command1 command "" [1ms]
 •  Executing command2 command ""  ...
 ✓  Executing command2 command "" [577013ns]
FAIL
FAIL	github.com/openshift/odo/pkg/devfile/adapters/common	0.205s
?   	github.com/openshift/odo/pkg/devfile/adapters/docker	[no test files]
ok  	github.com/openshift/odo/pkg/devfile/adapters/docker/component	(cached)
ok  	github.com/openshift/odo/pkg/devfile/adapters/docker/storage	(cached)
ok  	github.com/openshift/odo/pkg/devfile/adapters/docker/utils	(cached)
?   	github.com/openshift/odo/pkg/devfile/adapters/kubernetes	[no test files]
ok  	github.com/openshift/odo/pkg/devfile/adapters/kubernetes/component	(cached)
ok  	github.com/openshift/odo/pkg/devfile/adapters/kubernetes/storage	(cached)
ok  	github.com/openshift/odo/pkg/devfile/adapters/kubernetes/utils	(cached)
?   	github.com/openshift/odo/pkg/devfile/convert	[no test files]
ok  	github.com/openshift/odo/pkg/devfile/validate	(cached)
--- FAIL: TestValidateCommands (0.00s)
    --- FAIL: TestValidateCommands/Case_8:_Duplicate_commands (0.00s)
        commands_test.go:197: TestValidateAction unexpected error: <nil>
FAIL
FAIL	github.com/openshift/odo/pkg/devfile/validate/generic	0.089s
ok  	github.com/openshift/odo/pkg/envinfo	(cached)
ok  	github.com/openshift/odo/pkg/kclient	(cached)
?   	github.com/openshift/odo/pkg/kclient/fake	[no test files]
ok  	github.com/openshift/odo/pkg/lclient	(cached)
?   	github.com/openshift/odo/pkg/localConfigProvider	[no test files]
?   	github.com/openshift/odo/pkg/log	[no test files]
?   	github.com/openshift/odo/pkg/log/fidget	[no test files]
?   	github.com/openshift/odo/pkg/machineoutput	[no test files]
ok  	github.com/openshift/odo/pkg/notify	(cached) [no tests to run]
ok  	github.com/openshift/odo/pkg/occlient	(cached)
?   	github.com/openshift/odo/pkg/odo/cli	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/application	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/catalog	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/catalog/describe	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/catalog/list	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/catalog/search	[no test files]
ok  	github.com/openshift/odo/pkg/odo/cli/catalog/util	(cached)
?   	github.com/openshift/odo/pkg/odo/cli/component	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/component/ui	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/config	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/debug	[no test files]
ok  	github.com/openshift/odo/pkg/odo/cli/env	(cached)
?   	github.com/openshift/odo/pkg/odo/cli/login	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/logout	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/plugins	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/preference	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/project	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/registry	[no test files]
ok  	github.com/openshift/odo/pkg/odo/cli/registry/util	(cached)
ok  	github.com/openshift/odo/pkg/odo/cli/service	(cached)
ok  	github.com/openshift/odo/pkg/odo/cli/service/ui	(cached)
ok  	github.com/openshift/odo/pkg/odo/cli/storage	(cached)
?   	github.com/openshift/odo/pkg/odo/cli/ui	[no test files]
?   	github.com/openshift/odo/pkg/odo/cli/url	[no test files]
ok  	github.com/openshift/odo/pkg/odo/cli/utils	(cached)
?   	github.com/openshift/odo/pkg/odo/cli/version	[no test files]
?   	github.com/openshift/odo/pkg/odo/genericclioptions	[no test files]
ok  	github.com/openshift/odo/pkg/odo/util	(cached)
ok  	github.com/openshift/odo/pkg/odo/util/completion	(cached)
ok  	github.com/openshift/odo/pkg/odo/util/experimental	(cached)
ok  	github.com/openshift/odo/pkg/odo/util/pushtarget	(cached)
ok  	github.com/openshift/odo/pkg/odo/util/validation	(cached)
ok  	github.com/openshift/odo/pkg/preference	(cached)
ok  	github.com/openshift/odo/pkg/project	(cached)
ok  	github.com/openshift/odo/pkg/secret	(cached)
ok  	github.com/openshift/odo/pkg/service	(cached)
ok  	github.com/openshift/odo/pkg/storage	(cached)
ok  	github.com/openshift/odo/pkg/storage/labels	(cached)
ok  	github.com/openshift/odo/pkg/sync	(cached)
?   	github.com/openshift/odo/pkg/sync/mock	[no test files]
ok  	github.com/openshift/odo/pkg/testingutil	(cached)
?   	github.com/openshift/odo/pkg/testingutil/filesystem	[no test files]
ok  	github.com/openshift/odo/pkg/url	(cached)
ok  	github.com/openshift/odo/pkg/url/labels	(cached)
ok  	github.com/openshift/odo/pkg/util	(cached)
?   	github.com/openshift/odo/pkg/version	[no test files]
ok  	github.com/openshift/odo/pkg/watch	(cached)
FAIL
make: *** [Makefile:156: test] Error 1

pkg/config/config_test.go

dharmit · 2021-03-17T07:41:26Z

Holding since creating a service from outside the component directory is resulting in panic instead of a proper error message:

$ odo service create etcdoperator.v0.9.4-clusterwide/EtcdCluster 
Deploying service "example" of type: "EtcdCluster"
 ✗  Deploying service [4ms]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xb0 pc=0x1439e65]

goroutine 1 [running]:
github.com/openshift/odo/pkg/envinfo.(*EnvSpecificInfo).AddServiceToDevfile(0x0, 0xc0006c2000, 0xba, 0xc000677830, 0x7, 0xba, 0x0)
        /home/dshah/go/src/github.com/openshift/odo/pkg/envinfo/service.go:29 +0x125
github.com/openshift/odo/pkg/service.CreateOperatorService(0xc0005b99e0, 0x0, 0xc000677830, 0x7, 0xc000a30a2d, 0x18, 0xc000676d86, 0x7, 0xc000a30a20, 0xc, ...)
        /home/dshah/go/src/github.com/openshift/odo/pkg/service/service.go:117 +0x12e
github.com/openshift/odo/pkg/odo/cli/service.(*OperatorBackend).RunServiceCreate(0xc0004403c0, 0xc000835e40, 0x0, 0x0)
        /home/dshah/go/src/github.com/openshift/odo/pkg/odo/cli/service/backends.go:223 +0x2a3
github.com/openshift/odo/pkg/odo/cli/service.(*CreateOptions).Run(0xc000835e40, 0x0, 0x0)
        /home/dshah/go/src/github.com/openshift/odo/pkg/odo/cli/service/create.go:168 +0x43
github.com/openshift/odo/pkg/odo/genericclioptions.GenericRun(0x232a180, 0xc000835e40, 0xc00075f180, 0xc00076e950, 0x1, 0x1)
        /home/dshah/go/src/github.com/openshift/odo/pkg/odo/genericclioptions/runnable.go:31 +0x13c
github.com/openshift/odo/pkg/odo/cli/service.NewCmdServiceCreate.func1(0xc00075f180, 0xc00076e950, 0x1, 0x1)
        /home/dshah/go/src/github.com/openshift/odo/pkg/odo/cli/service/create.go:194 +0x5e
github.com/spf13/cobra.(*Command).execute(0xc00075f180, 0xc00076e930, 0x1, 0x1, 0xc00075f180, 0xc00076e930)
        /home/dshah/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:830 +0x2aa
github.com/spf13/cobra.(*Command).ExecuteC(0xc000612c80, 0x20beed0, 0xc00005c360, 0x3)
        /home/dshah/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:914 +0x2fb
github.com/spf13/cobra.(*Command).Execute(...)
        /home/dshah/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:864
main.main()
        /home/dshah/go/src/github.com/openshift/odo/cmd/odo/odo.go:67 +0x330

/hold

dharmit · 2021-03-17T11:39:52Z

Unit test failure:

 --- FAIL: TestGetCommandFromFlag (0.00s)
    --- FAIL: TestGetCommandFromFlag/Case_9:_Valid_devfile_with_invalid_composite_commands (0.00s)
        command_test.go:803: TestGetCommand unexpected error for command: build wantErr: true err: <nil>
 •  Executing command1 command ""  ...

 ✓  Executing command1 command "" [1ms]
 •  Executing command2 command ""  ...

 ✓  Executing command2 command "" [814921ns]
 •  Executing command1 command ""  ...

 ✓  Executing command1 command "" [1ms]
 •  Executing command2 command ""  ...

 ✓  Executing command1 command "" [2ms]

 ✓  Executing command2 command "" [853597ns]
FAIL
FAIL	github.com/openshift/odo/pkg/devfile/adapters/common	0.101s

dharmit · 2021-03-17T14:45:28Z

@kadel @mik-dass @girishramnani @valaparthvi

This PR is ready for review now. TBH, there was a lot of tangential stuff I had to address while working on this, so apologies in advance if I missed something obvious. 😄

Also, I have skipped adding integration tests but added unit tests. I'd prefer not to add more integration tests unless it's really, really required.

dharmit · 2021-03-18T05:58:32Z

Test failures are showing up at places that have not been modified by this PR.

/test v4.7-integration-e2e

pkg/envinfo/service.go

dharmit · 2021-03-18T08:46:18Z

@kadel @mik-dass @girishramnani @valaparthvi

This PR is ready for review now. TBH, there was a lot of tangential stuff I had to address while working on this, so apologies in advance if I missed something obvious. smile

Also, I have skipped adding integration tests but added unit tests. I'd prefer not to add more integration tests unless it's really, really required.

Things that still need to be addressed:

deletion of service catalog based service from devfile
fix Operator Hub integration tests to execute from component directory
move the functions to add and remove service info from devfile to a different package (Add and remove service info from devfile #4465 (review))

maysunfaisal

Some comments around new devfile/library funcs

pkg/envinfo/storage.go

dharmit · 2021-04-12T03:20:11Z

Link to logs.

error: error creating buildah builder: Error writing blob: error storing blob to file "/var/tmp/storage358802064/6": error happened during read: read tcp 10.130.41.37:41084->108.177.11.128:443: read: connection reset by peer

dharmit · 2021-04-12T03:20:20Z

/test v4.7-integration-e2e

Changed function name from `AddServiceToDevfile` to `AddKubernetesComponentToDevfile`. Similar name change for delete function. Also, moves it from `pkg/envinfo` to `pkg/service` package

"odo service create" without any arguments was panicking.

Fixing issues to play well with ConsentTelemetry being set to false before each test runs and with the change in this PR to execute service create and delete commands from the component's directory.

openshift-ci · 2021-04-12T11:14:33Z

@dharmit: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/v4.7-e2e-4x-psi	`a422681`	link	`/test v4.7-e2e-4x-psi`
ci/prow/psi-kubernetes-integration-e2e	`130d862`	link	`/test psi-kubernetes-integration-e2e`
ci/prow/psi-openshift-integration-e2e	`130d862`	link	`/test psi-openshift-integration-e2e`
ci/prow/psi-minishift-integration-e2e	`130d862`	link	`/test psi-minishift-integration-e2e`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

mik-dass

/lgtm

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation kind/code-refactoring labels Mar 1, 2021

openshift-ci-robot requested review from mik-dass and valaparthvi March 1, 2021 09:26

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Mar 1, 2021

dharmit mentioned this pull request Mar 4, 2021

odo service create should add operator CR to the devfile #4160

Closed

4 tasks

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Mar 11, 2021

dharmit changed the title ~~'odo service create' adds CRD to devfile~~ Add and remove service info from devfile Mar 15, 2021

dharmit marked this pull request as ready for review March 15, 2021 09:27

openshift-ci-robot 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 Mar 15, 2021

dharmit 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 Mar 15, 2021

dharmit commented Mar 17, 2021

View reviewed changes

pkg/config/config_test.go Show resolved Hide resolved

openshift-ci-robot 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 Mar 17, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Mar 17, 2021

dharmit removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Mar 17, 2021

maysunfaisal mentioned this pull request Mar 17, 2021

Revert Test Data for odo devfile/library#72

Closed

kadel reviewed Mar 18, 2021

View reviewed changes

pkg/envinfo/service.go Outdated Show resolved Hide resolved

dharmit 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 Mar 18, 2021

maysunfaisal suggested changes Mar 18, 2021

View reviewed changes

pkg/envinfo/storage.go Outdated Show resolved Hide resolved

pkg/envinfo/storage.go Show resolved Hide resolved

pkg/envinfo/storage.go Outdated Show resolved Hide resolved

openshift-ci-robot assigned maysunfaisal Mar 18, 2021

dharmit added 16 commits April 12, 2021 12:21

Add and remove service info from devfile

9bd5100

Changes function name and moves to service package

e47816a

Changed function name from `AddServiceToDevfile` to `AddKubernetesComponentToDevfile`. Similar name change for delete function. Also, moves it from `pkg/envinfo` to `pkg/service` package

Address Maysun's PR review comments

ce17ce7

Modify storage create and delete to use changes in "devfile/library"

7b99e9b

Check for length of containers and mount paths

2f07372

Don't ignore the errors; use t.Error

5a044a2

Fixes based on PR review

23340c4

Addressed Mrinal's review comments and fixed panic

33b0752

"odo service create" without any arguments was panicking.

Fix integration tests

2486d59

Fixing issues to play well with ConsentTelemetry being set to false before each test runs and with the change in this PR to execute service create and delete commands from the component's directory.

Removes wrongly commented code.

c6f3855

Integration test for service create and delete

36d56e1

Remove unnecessary newlines

168ed94

Check devfile existence before anything else

5166672

Common function to decide service provider backend

bbf3617

Remove a test spec and modify another

c9c73ad

Adds incorrectly removed test spec back

8a5b211

dharmit mentioned this pull request Apr 12, 2021

Calls to "WriteYamlDevfile" functoin need to be re-considered #4607

Closed

Correct the error message to expect

130d862

mik-dass approved these changes Apr 12, 2021

View reviewed changes

openshift-ci-robot assigned mik-dass Apr 12, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 12, 2021

openshift-merge-robot merged commit 0553847 into redhat-developer:master Apr 12, 2021

dharmit deleted the fix-4160-store-k8s-manifest-in-devfile branch April 13, 2021 12:02

dharmit mentioned this pull request Apr 30, 2021

odo catalog describe service for operators #4671

Merged

4 tasks

dharmit mentioned this pull request Aug 17, 2021

Refactor underlying code for "odo service" commands #4242

Closed

6 tasks

dharmit mentioned this pull request Aug 25, 2021

Refactor machine output code #4948

Merged

17 tasks

rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and remove service info from devfile #4465

Add and remove service info from devfile #4465

dharmit commented Mar 1, 2021 •

edited

Loading

openshift-ci-robot commented Mar 1, 2021

openshift-ci-robot commented Mar 1, 2021

dharmit commented Mar 15, 2021

dharmit commented Mar 17, 2021

dharmit commented Mar 17, 2021

dharmit commented Mar 17, 2021

dharmit commented Mar 18, 2021

dharmit commented Mar 18, 2021 •

edited

Loading

maysunfaisal left a comment

dharmit commented Apr 12, 2021 •

edited

Loading

dharmit commented Apr 12, 2021

openshift-ci bot commented Apr 12, 2021 •

edited

Loading

mik-dass left a comment

Add and remove service info from devfile #4465

Add and remove service info from devfile #4465

Conversation

dharmit commented Mar 1, 2021 • edited Loading

openshift-ci-robot commented Mar 1, 2021

openshift-ci-robot commented Mar 1, 2021

dharmit commented Mar 15, 2021

dharmit commented Mar 17, 2021

dharmit commented Mar 17, 2021

dharmit commented Mar 17, 2021

dharmit commented Mar 18, 2021

dharmit commented Mar 18, 2021 • edited Loading

maysunfaisal left a comment

Choose a reason for hiding this comment

dharmit commented Apr 12, 2021 • edited Loading

dharmit commented Apr 12, 2021

openshift-ci bot commented Apr 12, 2021 • edited Loading

mik-dass left a comment

Choose a reason for hiding this comment

dharmit commented Mar 1, 2021 •

edited

Loading

dharmit commented Mar 18, 2021 •

edited

Loading

dharmit commented Apr 12, 2021 •

edited

Loading

openshift-ci bot commented Apr 12, 2021 •

edited

Loading