Deploy, Events without devfile/adapters #5460

feloy · 2022-02-14T17:51:21Z

What type of PR is this:

/kind cleanup

What does this PR do / why we need it:

This PR rewrites the part of the devfile/adapters package used to execute commands, for the moment Deploy and Events commands, but can be completed to support other Commands (Build, Run, Test).

The content of the libdevfile package only cares about the devfile structure, and calls methods of the received handler to run the odo specific work. This way, devfile and odo code are clearly separated. The target is to include this code into the devfile/library, if devfile team agrees.

It also includes some convenient functions to create Command and Components of specific types (mostly convenient for writing tests).

The commands using this implementation:

the odo deploy and odo delete --deploy commands.
the odo push and odo delete commands parts dealing with postStart and preStop events.

It also moves some methods from the devfile/adapters package to the pkg/component package, as these methods are 100% component related and not devfile related.

Which issue(s) this PR fixes:

partially #5298
Fixes #5454

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:
Refer #5460 (comment) if you want to have the devfiles used for testing while working on the code for this PR.

netlify · 2022-02-14T17:51:26Z

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: 9f5923c

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

😎 Browse the preview: https://deploy-preview-5460--odo-docusaurus-preview.netlify.app

odo-robot · 2022-02-14T17:58:34Z

Unit Tests on commit 1431a1c finished successfully.
View logs: TXT HTML

odo-robot · 2022-02-14T18:05:57Z

OpenShift Tests on commit 1431a1c finished successfully.
View logs: TXT HTML

odo-robot · 2022-02-14T18:07:06Z

Validate Tests on commit 1431a1c finished successfully.
View logs: TXT HTML

odo-robot · 2022-02-14T18:48:55Z

Kubernetes Tests on commit 1431a1c finished with errors.
View logs: TXT HTML

odo-robot · 2022-02-14T19:50:40Z

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

dharmit

Reviewed only pkg/libdevfile/ yet.

pkg/libdevfile/libdevfile.go

pkg/libdevfile/command.go

pkg/libdevfile/component.go

dharmit

Only reviewed pkg/component/component.go.

dharmit · 2022-02-16T07:32:53Z

pkg/component/component.go

@@ -503,3 +509,87 @@ func setLinksServiceNames(client kclient.ClientInterface, linkedSecrets []Secret
 	}
 	return nil
 }
+


Among all the packages we have created the client interface for at business layer, component package has not been modified so far. Does it make sense to do it here?

The component one is the more complex to refactor. I'll need to sleep before ;) Joke aside, no, it will deserve PR all to itself

I'm going to need some sleep myself to form a mental picture of odo's code architecture. You're changing it fast (not complaining, complimenting), and it's tough to keep up at times. 😅

dharmit · 2022-02-16T09:56:15Z

@feloy maybe it's just me, but an example devfile that could be used to test the changes here by stepping through the code would be helpful because the PR is overall about refactoring and introducing a different (new?) code architecture. Do you know of a devfile I can use right away? Preferably a complete devfile, and not something in bits and pieces.

feloy · 2022-02-16T10:41:26Z

@feloy maybe it's just me, but an example devfile that could be used to test the changes here by stepping through the code would be helpful because the PR is overall about refactoring and introducing a different (new?) code architecture. Do you know of a devfile I can use right away? Preferably a complete devfile, and not something in bits and pieces.

I've used this one to test the events: https://github.com/redhat-developer/odo/blob/main/tests/examples/source/devfiles/nodejs/devfile-with-valid-events.yaml
And this one for deploy/undeploy: https://github.com/redhat-developer/odo/blob/main/tests/examples/source/devfiles/nodejs/devfile-deploy.yaml

valaparthvi · 2022-02-16T15:08:14Z

pkg/libdevfile/command.go

+type command interface {
+	CheckValidity() error
+	Execute(handler Handler) error
+	UnExecute() error


This method is no longer needed it seems. I remember adding it to implement undeploy, but since Execute of undeployHandler handles the undeployment part, we can remove this method, IMO.

You are right, good catch

valaparthvi · 2022-02-16T15:09:11Z

pkg/libdevfile/command.go

+		cmd = newExecCommand(devfileObj, devfileCmd)
+	}
+
+	if err := cmd.CheckValidity(); err != nil {


Suggested change

if err := cmd.CheckValidity(); err != nil {

if err = cmd.CheckValidity(); err != nil {

valaparthvi · 2022-02-17T04:54:25Z

pkg/component/exec_handler.go

+}
+
+func getCmdline(command v1alpha2.Command) []string {
+	exe := command.Exec


Perhaps you can get rid off this variable?

valaparthvi · 2022-02-17T05:29:29Z

pkg/libdevfile/libdevfile.go

+		}
+		return foundGroupCmd, nil
+	}
+	return groupCmds[0], nil


This returns the command even if it might not be default. I assume we consider that if only one command is present, take it as default?

Yes, that's assumed (it is explained in the comment of the function).

valaparthvi · 2022-02-17T05:43:37Z

pkg/libdevfile/command_apply.go

+		return fmt.Errorf("component %q does not exists", o.command.Apply.Component)
+	}
+
+	if len(devfileComponents) != 1 {
+		return fmt.Errorf("more than one component with the same name, should not happen")


Thoughts on adding error structs for these errors?

I added these, thanks

valaparthvi · 2022-02-23T05:44:29Z

The tests pass, so approving it, and the code looks good for now.
/lgtm
/approve

openshift-ci · 2022-02-23T05:44:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: valaparthvi

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

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

…kage

sonarqubecloud · 2022-02-23T07:40:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
1.9% Duplication

valaparthvi · 2022-02-23T08:46:55Z

/lgtm

* Execute devfile command * Undeploy * cleanup devfile/adapters * refactor * Move GetOnePod to component package * Move DoesComponentExist and Log from devfile/adapter to component package * Exec without devfile/adapters * Move Delete from devfile/adapters to component * Remove old Deploy code * review * Add tests for issue 5454 * Review

openshift-ci bot added the kind/cleanup label Feb 14, 2022

openshift-ci bot requested review from dharmit and rnapoles-rh February 14, 2022 17:51

feloy requested review from valaparthvi, kadel and cdrage and removed request for rnapoles-rh February 14, 2022 17:51

feloy force-pushed the without-devfile-adapters branch from d5e9f17 to 51dacab Compare February 15, 2022 12:31

feloy changed the title ~~Deploy without devfile/adapters~~ Deploy, Events without devfile/adapters Feb 15, 2022

feloy force-pushed the without-devfile-adapters branch 2 times, most recently from 231ceb6 to f867c10 Compare February 15, 2022 15:10

dharmit reviewed Feb 16, 2022

View reviewed changes

valaparthvi reviewed Feb 17, 2022

View reviewed changes

feloy requested review from valaparthvi and dharmit February 17, 2022 11:09

openshift-ci bot assigned valaparthvi Feb 23, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Feb 23, 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 23, 2022

Execute devfile command

05656bd

feloy added 11 commits February 23, 2022 08:34

Undeploy

b2ce427

cleanup devfile/adapters

ae7f800

refactor

9b06644

Move GetOnePod to component package

a6b8f40

Move DoesComponentExist and Log from devfile/adapter to component pac…

049a9f8

…kage

Exec without devfile/adapters

cd3a4ed

Move Delete from devfile/adapters to component

d5ef2dd

Remove old Deploy code

a140294

review

740b20f

Add tests for issue 5454

669f5c1

Review

9f5923c

feloy force-pushed the without-devfile-adapters branch from 86536d5 to 9f5923c Compare February 23, 2022 07:39

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

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

openshift-merge-robot merged commit 0e4e55b into redhat-developer:main Feb 23, 2022

feloy mentioned this pull request Feb 23, 2022

2nd non-default deploy command defined in devfile causes odo to report error #5452

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deploy, Events without devfile/adapters #5460

Deploy, Events without devfile/adapters #5460

feloy commented Feb 14, 2022 •

edited

Loading

netlify bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

dharmit left a comment

dharmit left a comment

dharmit Feb 16, 2022

feloy Feb 16, 2022

dharmit Feb 16, 2022

dharmit commented Feb 16, 2022

feloy commented Feb 16, 2022

valaparthvi Feb 16, 2022

feloy Feb 17, 2022

valaparthvi Feb 16, 2022

valaparthvi Feb 17, 2022

valaparthvi Feb 17, 2022

feloy Feb 17, 2022 •

edited

Loading

valaparthvi Feb 17, 2022

feloy Feb 17, 2022

valaparthvi commented Feb 23, 2022

openshift-ci bot commented Feb 23, 2022

sonarqubecloud bot commented Feb 23, 2022

valaparthvi commented Feb 23, 2022

	if err := cmd.CheckValidity(); err != nil {
	if err = cmd.CheckValidity(); err != nil {

Deploy, Events without devfile/adapters #5460

Deploy, Events without devfile/adapters #5460

Conversation

feloy commented Feb 14, 2022 • edited Loading

netlify bot commented Feb 14, 2022 • edited Loading

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

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

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

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

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

dharmit left a comment

Choose a reason for hiding this comment

dharmit 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

dharmit commented Feb 16, 2022

feloy commented Feb 16, 2022

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 17, 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

openshift-ci bot commented Feb 23, 2022

sonarqubecloud bot commented Feb 23, 2022

valaparthvi commented Feb 23, 2022

feloy commented Feb 14, 2022 •

edited

Loading

netlify bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

odo-robot bot commented Feb 14, 2022 •

edited

Loading

feloy Feb 17, 2022 •

edited

Loading