Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Having separate layers #5247

Closed
4 of 15 tasks
feloy opened this issue Nov 24, 2021 · 3 comments
Closed
4 of 15 tasks

Having separate layers #5247

feloy opened this issue Nov 24, 2021 · 3 comments
Assignees
Labels
area/refactoring Issues or PRs related to code refactoring priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).

Comments

@feloy
Copy link
Contributor

feloy commented Nov 24, 2021

/kind bug

As described in the comment #4057, the architecture of the code should use several layers (bottom to top):

  • low-level layer that interacts with the cluster (using k8s.io packages)
  • business layer that defines abstractions of the domain (app, component, project, storage, url, etc)
  • cli layer that defines the UI

Packages in layers

The low-level layer is composed of packages that access the underlying platforms:

  • pkg/occlient
  • pkg/kclient
  • pkg/devfile (?)

The business layer is composed of packages that manage the odo objects:

The CLI layer is composed of packages in:

  • cmd
  • pkg/odo/cli

Other packages:

  • pkg/segment

Import of CLI layer

Packages of the CLI layer should not be imported from outside this CLI layer. These imports should be fixed:

$ grep -r github.com/openshift/odo/pkg/odo/cli pkg/ | grep -v ^pkg/odo/cli

pkg/odo/genericclioptions/runnable.go:			"github.com/openshift/odo/pkg/odo/cli/ui"
pkg/preference/preference.go:				"github.com/openshift/odo/pkg/odo/cli/ui"
pkg/catalog/catalog.go:			registryConsts	"github.com/openshift/odo/pkg/odo/cli/registry/consts"
pkg/catalog/catalog.go:			registryUtil	"github.com/openshift/odo/pkg/odo/cli/registry/util"
pkg/component/starter_project.go:	registryUtil	"github.com/openshift/odo/pkg/odo/cli/registry/util"

Interactions with the console

Interactions with user (and especially console) should be done only from the CLI layer.

$ grep -r fmt.Print pkg/ | grep -v ^pkg/odo/cli | grep -v _test.go:

pkg/testingutil/httputils.go:	fmt.Println("running")
pkg/log/fidget/spinner.go:	fmt.Print("\r")
pkg/auth/login.go:		fmt.Println("Login failed (401 Unauthorized)")
pkg/auth/login.go:		fmt.Println("Verify you have provided correct credentials.")
pkg/auth/login.go:		fmt.Println(cause.Message)

Interactions with the cluster

Interactions with the cluster should be done only from the low-level layer.

$ grep -r client-go pkg/ |grep -v ^pkg/kclient | grep -v ^pkg/occlient | grep -v _test.go:

pkg/testingutil/configs.go:	clientcmdapi 	"k8s.io/client-go/tools/clientcmd/api"
pkg/debug/portforward.go:			"k8s.io/client-go/rest"
pkg/debug/portforward.go:			"k8s.io/client-go/tools/portforward"
pkg/debug/portforward.go:			"k8s.io/client-go/transport/spdy"
pkg/auth/login.go:				"k8s.io/client-go/tools/clientcmd"
pkg/util/util.go:				"k8s.io/client-go/util/homedir"

Import of low-level layer

Packages of the low-level layer should be imported from business layer only.

Interfaces

Packages in the business layer and the low-level layer should declare an interface and a default Client implementing this interface so the upper layers use this interface and either use the default implementation, or create its own implementation for testing.

(to be continued)

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 24, 2021
@kadel kadel added kind/design priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). and removed kind/bug Categorizes issue or PR as related to a bug. labels Nov 25, 2021
@kadel
Copy link
Member

kadel commented Nov 25, 2021

This is great. Thank you, @feloy, for writing this down 👍

The low-level layer is composed of packages in:

  • pkg/occlient
  • pkg/kclient

Another thing that I would to see is consolidation of occlient and kclient into one package. There is a lot of duplication between those two, and with recent move to removing s2i related code from odo there are only 2 things that are specific to OpenShift that odo uses (Routes and Projects). Technically even those two are just another Kubernetes resources that are not different from any other Kubernetes CRs. Odo should treat all resources the same way.

@feloy
Copy link
Contributor Author

feloy commented Nov 25, 2021

This is great. Thank you, @feloy, for writing this down +1

The low-level layer is composed of packages in:

  • pkg/occlient
  • pkg/kclient

Another thing that I would to see is consolidation of occlient and kclient into one package. There is a lot of duplication between those tow, and with recent move to removing s2i related code from odo there are only 2 things that are specific to OpenShift that odo uses (Routes and Projects). Technically even those two are just another Kubernetes resources that are not different from any other Kubernetes CRs. Odo should treat all resources the same way.

Thanks :)
I created another issue for merging occlient/kclient: #5249

@dharmit dharmit added this to For Consideration in Sprint 211 via automation Dec 2, 2021
@dharmit dharmit moved this from For Consideration to To Do in Sprint 211 Dec 2, 2021
@feloy feloy moved this from To Do to In Progress in Sprint 211 Dec 14, 2021
@feloy feloy moved this from In Progress to For Review in Sprint 211 Dec 16, 2021
@dharmit dharmit added this to For Consideration in Sprint 212 via automation Dec 20, 2021
@dharmit dharmit moved this from For Consideration to To Do in Sprint 212 Dec 22, 2021
@dharmit dharmit moved this from For Review to Done in Sprint 211 Dec 22, 2021
@feloy feloy moved this from To Do to In Progress in Sprint 212 Jan 7, 2022
@feloy feloy moved this from In Progress to For Review in Sprint 212 Jan 10, 2022
@joshira-rh joshira-rh added this to For Consideration in Sprint 213 via automation Jan 12, 2022
@joshira-rh joshira-rh removed this from For Review in Sprint 212 Jan 12, 2022
This was referenced Jan 13, 2022
@feloy
Copy link
Contributor Author

feloy commented Jan 31, 2022

Remaining packages will be deleted or worked on for v3

@feloy feloy closed this as completed Jan 31, 2022
Sprint 213 automation moved this from For Consideration to Done Jan 31, 2022
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/refactoring Issues or PRs related to code refactoring priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects
Archived in project
Sprint 211
  
Done
Sprint 213
  
Done
Development

No branches or pull requests

4 participants