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

Regroup the command implementations in respective sub-packages #66

Closed
navidshaikh opened this issue Apr 9, 2019 · 6 comments
Closed

Comments

@navidshaikh
Copy link
Collaborator

Presently, all the command implementations reside under pkg/kn/commands/, we should re-group the implementations based on command-group.

for eg:

pkg/kn/commands/service/ <-- implementing all the sub-commands under kn service
pkg/kn/commands/revision/
pkg/kn/commands/build/
pkg/kn/commands/

and so on..

@maximilien
Copy link
Contributor

maximilien commented May 13, 2019

So I took at pass at this when I was doing the plugin command. It's doable but requires the following changes:

  1. move some types from root.go to a common package to avoid import cycles, e.g., this common package for one of my plugin branches

  2. sweeping change to each command to use the common package above, e.g., this plugins example

Bottom line is that it's not hard to do just requires agreement and one swoop change.

If you guys want this before milestone 1 I could do this this week, while you all asleep in Europe. I'll add this to the agenda tomorrow as a low priority issue.

/cc @sixolet @cppforlife

@navidshaikh
Copy link
Collaborator Author

IMO, we should aim this as first thing after v0.1.

@rhuss
Copy link
Contributor

rhuss commented May 28, 2019

@maximilien Is this something you are already on ? I'm asking while I'm working on #14 I also want to split up and separate the commands better (as I want to use some some package scoped vars which shouldn't 'pollute' whole "commands").

I agree that we need a "common" package which should hold the shared code, e.g. KnParams. I also believe that KnParams should be an interface defined in common and an implementation in the root "commands" package.

As I'm on it as well, I would be happy also to take over this task if you don't have a chance to come to is soonish. (there are some other subtle things like how to register subcommands and also the human-readable printers now reference commands directly).

In any case, we should tackle this issue very soon.

@maximilien
Copy link
Contributor

OK as per discussion today. Let me submit something today. I am sure you guys will have lots of feedback. More soon.

maximilien added a commit to maximilien/client that referenced this issue May 28, 2019
Adds three new packages:

1. kn/commands/common - includes common stuff between commands
2. kn/commands/service - includes all kn service * commands
3. kn/commands/revision - includes all kn revision * commands

Overall worked fine. I have two things I don't like but that
I think should be done as refactor. I'll mention them in
comments for discussions.
maximilien added a commit to maximilien/client that referenced this issue May 28, 2019
Creates three subpackages for now:

1. kn/commands/common - inludes types.go for common
   struct. Also includes humman_readable_flags.go
   which could be split into the specific command
   package. But I would do this as a refactor later
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
maximilien added a commit to maximilien/client that referenced this issue May 28, 2019
Creates three subpackages for now:

1. kn/commands/common - inludes types.go for common
   struct and other common files
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
@maximilien
Copy link
Contributor

Done in #145

maximilien added a commit to maximilien/client that referenced this issue May 29, 2019
Creates four subpackages for now:

1. kn/commands/common - inludes types.go for common
   struct and other common files
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing
maximilien added a commit to maximilien/client that referenced this issue May 29, 2019
Creates four subpackages for now:

1. kn/commands/common - inludes types.go for common
   struct and other common files
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing
maximilien added a commit to maximilien/client that referenced this issue May 29, 2019
Creates four subpackages for now:

1. kn/commands/common - inludes types.go for common
   struct and other common files
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing

Had to refactor all tests to avoid cycles.
maximilien added a commit to maximilien/client that referenced this issue May 30, 2019
Creates four subpackages for now:

1. kn/commands/common - inludes types.go for common
   struct and other common files
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing

Had to refactor all tests to avoid cycles.
@rhuss rhuss mentioned this issue Jun 2, 2019
maximilien added a commit to maximilien/client that referenced this issue Jun 4, 2019
Creates four subpackages for now:

1. kn/commands - inludes types.go for common
   struct and other common files and misc commands
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing
5. refactors:
   a. split .../commons/human_readable_flags.go into three
   b. modifies the HumanReadableFlags.ToPrinter to get pass
      a function that sets the columns and fields

Had to refactor all tests to avoid cycles.
maximilien added a commit to maximilien/client that referenced this issue Jun 5, 2019
Creates four subpackages for now:

1. kn/commands - inludes types.go for common
   struct and other common files and misc commands
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing
5. refactors:
   a. split .../commons/human_readable_flags.go into three
   b. modifies the HumanReadableFlags.ToPrinter to get pass
      a function that sets the columns and fields

Had to refactor all tests to avoid cycles.
maximilien added a commit to maximilien/client that referenced this issue Jun 5, 2019
Creates four subpackages for now:

1. kn/commands - inludes types.go for common
   struct and other common files and misc commands
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing
5. refactors:
   a. split .../commons/human_readable_flags.go into three
   b. modifies the HumanReadableFlags.ToPrinter to get pass
      a function that sets the columns and fields

Had to refactor all tests to avoid cycles.
knative-prow-robot pushed a commit that referenced this issue Jun 5, 2019
Creates four subpackages for now:

1. kn/commands - inludes types.go for common
   struct and other common files and misc commands
2. kn/commands/service - all 'kn service *' commands
3. kn/commands/revision - all 'kn revision *' commands
4. kn/core - contains the root.go and other top level
   for code and testing
5. refactors:
   a. split .../commons/human_readable_flags.go into three
   b. modifies the HumanReadableFlags.ToPrinter to get pass
      a function that sets the columns and fields

Had to refactor all tests to avoid cycles.
@navidshaikh
Copy link
Collaborator Author

This is completed, thanks @maximilien !

coryrc pushed a commit to coryrc/client that referenced this issue May 14, 2020
* Add a function to check links in markdown files

Bonus: make header/subheader nicer and factor out banner creation.

* Fix and add comments

* Document make_banner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants