redhat-developer / opencompose

OpenCompose - A higher level abstraction for Kubernetes Resource
Apache License 2.0
64 stars 12 forks source link

Refactor CMD / cobra implementation #168

Closed cdrage closed 4 years ago

cdrage commented 7 years ago

It's confusing that we have a cmd package that is then referenced / re-implemented elsewhere in the pkg/cmd folder.

There are parts in the code that do not follow the cobra / CLI standards such as https://github.com/redhat-developer/opencompose/blob/master/pkg/cmd/util/helpers.go where it is four folder deep as well as confusing functionalities as well as unorthodox / non-standard usage-error outputs: https://github.com/redhat-developer/opencompose/blob/master/pkg/cmd/util/helpers.go#L19

A few things:

Current issues:

kadel commented 7 years ago

Move pkg/cmd back to just cmd. This is a common folder for all things CLI in projects which implement cobra (see https://github.com/kubernetes-incubator/kompose/tree/master/cmd https://github.com/docker/cli/tree/master/cli https://github.com/giantswarm/cli/tree/master/cli) However, some projects (such as OpenShift) have it in pkg/cmd, however, this is only one of two examples I found over 20-ish implentations where it's in a separate pkg folder. Or move in a separate folder all-together (see https://github.com/rkt/rkt/tree/master/rkt) or cli.

Are there any real benefits for moving it? I don't think it is worth refactoring something just for the sake of refactoring it, or that some other projects do it differently.

Refactor the CMD implementation to something much simpler. Take code out such as https://github.com/redhat-developer/opencompose/blob/master/pkg/cmd/convert.go#L63 and https://github.com/redhat-developer/opencompose/blob/master/pkg/cmd/convert.go#L116 into it's own separate functions / "transformer" folder. These cmd files are very long and obviously were used to get the initial POC working.

I don't disagree with you, but I don't think this is worth doing unless there is some real need for doing it.

Some examples aren't very intuitite / informative, see ./opencompose version and how as an example, it says kubectl version.

Don't understand that you mean :-(

--distro should be provider (kubernetes are openshift aren't distros?)

I like --distro much more than provider. OpenCompose will always use Kuberentes as default. OpenShift is a distribution of Kubernetes. It makes much more sense to use --distro fort.

cdrage commented 7 years ago

@kadel Ideally, it's my gripe with the unconventional implentation of Cobra, having a lot of functionality happen in the cmd folder which should be refactored into a separate folder. That's my only issue.

It works fine! Don't get me wrong! But going through some of the Cobra code I found that I had to jump between 3 files to find out how it's functioning correctly.