Regroups code to use subpackages. Finishes issue #66#145
Regroups code to use subpackages. Finishes issue #66#145knative-prow-robot merged 1 commit intoknative:masterfrom
Conversation
knative-prow-robot
added
the
size/XL
|
/assign @sixolet @cppforlife OK so as promised this regroups things. Overall it should be a good start. I have two issues with it that we can solve later during refactors. If you agree. Here they are:
rootCmd.PersistentFlags().StringVar(&common.KubeCfgFile, "kubeconfig", "", "kubectl config file (default is $HOME/.kube/config)")without having to make it public? Might have to have two vars and sync them, which is also not nice.
Anyhow, let me know your thoughts. Also need to add tests for |
|
Thanks a lot ! I will have a look tomorrow (hopefully), sorry won't get to it today. |
|
BTW, tests (unit and integration) are failing. |
Still making changes. Tests failures require some more fiddling with packages. Wait for next update. |
sixolet
left a comment
sixolet
left a comment
There was a problem hiding this comment.
I think the main thing I'm worried about is division of responsiblity for flags.
maximilien
force-pushed
the
subpackages
branch
3 times, most recently
from
4ac1c77 to
27a8537
Compare
|
OK all. Current version does pass tests. I had to refactor all tests to use an additional The end result works. The code is tiddy and well packaged and done (except for the refactoring I need to do for the The tests suffered a bit since I had to duplicate some code in the test version of the Also we now have two packages ( Let me know your thoughts: @cppforlife @sixolet @rhuss |
|
/test pull-knative-client-integration-tests |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
The PR looks good to me, nice job @maximilien !
I have some minor comments about naming and the final structure, though:
- Why do we need the
kndirectory level belowpkg/? I think "commands" can be moved up one level andknremoved. - Can't we put "root.go", "completion.go" and "version.go" directly into the "commands" package ? This would get rid of the 'etc/' and 'core/' directories/packages.
- Can we please rename the package for service and revision to
command_revisionandcommand_service(or cmd_revision / cmd_service). This would make it clear from the package name that its about a command and dedicated libs (without references to cobra commands) can be named like 'serving' and 'eventing' without adding to much of confusion.
After this bigger restructuring, we can still fine-tune and e.g. refactor the utils/test_helper.go which imo shouldn't be there, too.
As another refactoring tasks which I see after this move are:
- Move as much functionality as possible out of the commands in cobra independent libraries (i.e. into the serving package) so that they can be reused easily. E.g. 'KnServiceCreateCommand' contains too much logic (which even tends to grow e.g. when #156 is added). It should be only a thin layer of a more generic lib imo
- Refactor tests to become more unit tests. E.g. commands should be tested only that they propagate the proper info to the library calls (e.g. in serving), the library calls should then use the serving mock library. Only a suggestion, but tests currently are not easy to maintain.
But as mentioned, I would be very happy if this PR gets merged soonish.
|
@maximilien The decoupling works nicely, just as a comment for another approach which might be useful, too, would be to reverse the direction of the commands registration (i.e. letting sub-commands register themselves). It's more a syntactic thing, and I think it can also break dep cycles more easily (maybe even without the introduction of third That's the idea (not fully tested):
package commands
// That unnamed import is crucial here
import _ cmd_service
typ KnParams struct {
// ...
}
var SubCommandFactories = make([]func(*KnParams) *cobra.Command,0)
func NewKnCommand(params ...common.KnParams) *cobra.Command {
for _, fact := range SubCommandFactories {
rootCmd.AddCommand(fact.newCommand(p))
}
}
package cmd_service
import commands
func init() {
commands.SubCommandFactories = append(commands.SubCommandFactories, newServiceCreateFactor)
}
func newServiceCreateFactory(p *commands.KnParams) *cobra.Command {
////
}You then can easily add new commands by adding an unnamed import which is used to call its init() method. I've seen that pattern in the wild quite a bit (I darkly remember its used also for collecting k8s resource schemes), so might make sense here, too. |
Let's explore this after we agree on the package naming and whether Also, most importantly, I need to resolve various conflicts as we merge the PRs newer to this one... Best. |
|
OK @sixolet and @rhuss this last pushed code addresses almost everything that we discussed today. Missing are:
Let me know if you are OK with this so I can rebase and submit last version of this PR :) cc: @cppforlife did not address your test comment. Will do so in #161 |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
Cool, thanks a ton @maximilien ! Lgtm (with some minor nit-picks), I'm good with going forward.
| return &HumanPrintFlags{} | ||
| } | ||
|
|
||
| // Private functions |
There was a problem hiding this comment.
When the following functions are private, why are the names uppercased ? (very likely in the original code, so not really part of the PR. nevertheless ;-)
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package commands |
There was a problem hiding this comment.
I would call the file helper_test.go so that it gets compiled and used only during testing.
|
@maximilien btw, rebasing on upstream/master was surprisingly easy, only some minor conflicts in import statements. I attach a patch with the rebased changed (could be applied on top of your branch) for your reference, but wouldn't apply it (just as a reference) as you would come into troubles with a rebase. I could push my rebased branch to yours (if you allow), but I'm pretty sure doing the rebase yourself is easy, too. Thanks again for the quick update! |
|
Thanks @rhuss that makes me looking forward to it now :) Will do it in a few hours. Got a few company stuff to complete in the AM. More soon. Best, Max |
|
Looks like a back end infra failure /test pull-knative-client-unit-tests |
|
/retest |
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.
|
So right @rhuss, rebasing was a breeze 🏖 All green so good to merge @sixolet and @cppforlife |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maximilien, rhuss, sixolet The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
Remove unused scripts
7 participants
Creates three subpackages for now:
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