Separate layers - Application + Project #5293

feloy · 2021-12-15T07:26:04Z

What type of PR is this?

/kind cleanup

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Part of #5247

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

netlify · 2021-12-15T07:26:09Z

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

🔨 Explore the source changes: 05acfba

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

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

feloy · 2021-12-15T08:16:44Z

Kubernetes Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T08:26:16Z

Unit Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T08:31:13Z

OpenShift Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T10:49:53Z

Kubernetes Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T11:21:44Z

Unit Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T11:35:12Z

OpenShift Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T14:02:34Z

Unit Tests finished with errors.
View logs: TXT HTML

feloy · 2021-12-15T14:08:23Z

Kubernetes Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T14:10:27Z

OpenShift Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T15:33:15Z

Unit Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-15T15:51:48Z

OpenShift Tests finished with errors.
View logs: TXT HTML

feloy · 2021-12-15T16:01:50Z

Kubernetes Tests finished with errors.
View logs: TXT HTML

kadel · 2021-12-17T10:18:49Z

This is great 👍 I like the approach with Client interfaces

/approve

openshift-ci · 2021-12-17T10:18:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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

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

dharmit

I understand that at times I have requested/suggested changes in the piece of code that you have only copied over. I have done that since we are sort of refactoring the bits here. If you think we should handle them separately, we can discuss.

dharmit · 2021-12-24T08:59:10Z

pkg/application/kubernetes.go

@@ -0,0 +1,111 @@
+package application


Why a separate kubernetes.go? Are we going to have a openshift.go at some point? Also, #5247 (comment).

Not particularly, this is to separate the implementation from the definition of the interface, and to make something similar to url and storage.

dharmit · 2021-12-24T09:02:29Z

pkg/odo/cli/application/describe.go

 }

 // NewCmdDescribe implements the odo command.
 func NewCmdDescribe(name, fullName string) *cobra.Command {
-	o := NewDescribeOptions()
+	kubclient, _ := kclient.New()


Suggested change

kubclient, _ := kclient.New()

// The error is not handled at this point, it will be handled during Context creation

kubclient, _ := kclient.New()

dharmit · 2021-12-24T09:02:46Z

pkg/odo/cli/application/list.go

 }

 // NewCmdList implements the odo command.
 func NewCmdList(name, fullName string) *cobra.Command {
-	o := NewListOptions()
+	kubclient, _ := kclient.New()


Suggested change

kubclient, _ := kclient.New()

// The error is not handled at this point, it will be handled during Context creation

kubclient, _ := kclient.New()

pkg/application/kubernetes.go

dharmit · 2021-12-24T10:14:22Z

pkg/application/kubernetes.go

+}
+
+// List all applications names in current project by looking at `app` labels in deployments
+func (o kubernetesClient) List() ([]string, error) {


Thinking out loud...

Should this function and other List functions in business logic return a struct (AppList in this case) which can be used as-is if the user requested JSON output; and if the user requested human-readable output, the CLI layer could parse things?

I'm thinking this way because:

IIUC, k8s does this by default, in the sense that kubectl gets a big JSON which it parses to print human-readable output.

Our business layer could act as an API provided we don't break it.

That would an interesting subject for another refactoring

Sure. Let's discuss this alongside the v3 plans.

pkg/project/kubernetes.go

openshift-ci · 2022-01-05T13:53:04Z

@feloy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/psi-unit-test-windows	`05acfba`	link	false	`/test psi-unit-test-windows`
ci/prow/psi-unit-test-mac	`05acfba`	link	false	`/test psi-unit-test-mac`

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.

odo-robot · 2022-01-05T13:56:19Z

Unit Tests on commit 27b892c finished successfully.
View logs: TXT HTML

odo-robot · 2022-01-05T13:59:40Z

OpenShift Tests on commit 27b892c finished successfully.
View logs: TXT HTML

dharmit · 2022-01-05T14:04:20Z

/lgtm

odo-robot · 2022-01-05T14:07:21Z

Kubernetes Tests on commit 27b892c finished successfully.
View logs: TXT HTML

openshift-ci bot added the kind/cleanup label Dec 15, 2021

feloy changed the title ~~Refactor/application on main~~ Separate layers - Application Dec 15, 2021

openshift-ci bot requested review from mohammedzee1000 and rnapoles-rh December 15, 2021 07:26

feloy mentioned this pull request Dec 15, 2021

[wip] Separate layers - Application #5285

Closed

4 tasks

feloy force-pushed the refactor/application-on-main branch from bf7541c to 1061fe5 Compare December 15, 2021 13:45

feloy force-pushed the refactor/application-on-main branch from 1061fe5 to 07d6d2d Compare December 15, 2021 15:09

feloy changed the title ~~Separate layers - Application~~ Separate layers - Application + Project Dec 15, 2021

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

This was referenced Dec 20, 2021

Having separate layers #5247

Closed

Separate layers : Preference #5296

Merged

dharmit suggested changes Dec 24, 2021

View reviewed changes

openshift-ci bot assigned dharmit Dec 24, 2021

feloy added 2 commits January 5, 2022 14:01

Application interface

d41dc7b

Application describe

f236a6f

feloy added 10 commits January 5, 2022 14:01

Application list

4be974f

Fix --output/-o flag

15d8630

Test Run()

004929e

Tests on application pkg

61a5767

Unit tests on kclient relative to application

9b0272a

comment

6068591

Add ComponentList method to Application

d2beacb

Project interface

cbebf11

Project CLI tests

f919d46

Dharmit review

05acfba

feloy force-pushed the refactor/application-on-main branch from 07d6d2d to 05acfba Compare January 5, 2022 13:29

feloy requested a review from dharmit January 5, 2022 13:47

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

openshift-merge-robot merged commit b20103c into redhat-developer:main Jan 5, 2022

dharmit mentioned this pull request Jan 6, 2022

Business logic to act as API layer? #5317

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

Separate layers - Application + Project #5293

Separate layers - Application + Project #5293

feloy commented Dec 15, 2021 •

edited

Loading

netlify bot commented Dec 15, 2021 •

edited

Loading

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

kadel commented Dec 17, 2021

openshift-ci bot commented Dec 17, 2021

dharmit left a comment

dharmit Dec 24, 2021

feloy Jan 5, 2022

dharmit Dec 24, 2021

dharmit Dec 24, 2021

dharmit Dec 24, 2021

feloy Jan 5, 2022

dharmit Jan 5, 2022

openshift-ci bot commented Jan 5, 2022 •

edited

Loading

odo-robot bot commented Jan 5, 2022

odo-robot bot commented Jan 5, 2022

dharmit commented Jan 5, 2022

odo-robot bot commented Jan 5, 2022

	kubclient, _ := kclient.New()
	// The error is not handled at this point, it will be handled during Context creation
	kubclient, _ := kclient.New()

Separate layers - Application + Project #5293

Separate layers - Application + Project #5293

Conversation

feloy commented Dec 15, 2021 • edited Loading

netlify bot commented Dec 15, 2021 • edited Loading

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

feloy commented Dec 15, 2021

kadel commented Dec 17, 2021

openshift-ci bot commented Dec 17, 2021

dharmit left a comment

Choose a reason for hiding this comment

dharmit Dec 24, 2021

Choose a reason for hiding this comment

feloy Jan 5, 2022

Choose a reason for hiding this comment

dharmit Dec 24, 2021

Choose a reason for hiding this comment

dharmit Dec 24, 2021

Choose a reason for hiding this comment

dharmit Dec 24, 2021

Choose a reason for hiding this comment

feloy Jan 5, 2022

Choose a reason for hiding this comment

dharmit Jan 5, 2022

Choose a reason for hiding this comment

openshift-ci bot commented Jan 5, 2022 • edited Loading

odo-robot bot commented Jan 5, 2022

odo-robot bot commented Jan 5, 2022

dharmit commented Jan 5, 2022

odo-robot bot commented Jan 5, 2022

feloy commented Dec 15, 2021 •

edited

Loading

netlify bot commented Dec 15, 2021 •

edited

Loading

openshift-ci bot commented Jan 5, 2022 •

edited

Loading