WIP: refactor: place utility methods in pkg/cmd #1371

rkpattnaik780 · 2022-01-06T15:37:32Z

Moved utility methods to respective commands.

The sub-directories of kafkautil could be moved to the respective command specific directory.
Certain methods of kafkautil, serviceregistryutil and serviceaccountutil had few methods used by core, specific methods have been moved to respective directory.

Directory Structure

|─ pkg
|── cmd
|   ├── kafka
|           |─── create
|           |─── update
|           |─── topic
|                   |─── create
|                   |─── update
|                   |─── ...
|                   |─── sdk
|           |─── ......
|           |─── sdk
|   ├── cluster
|           |─── sdk
|           |─── ...
|   ├── sdk
|──  kafkautil
|── serviceregistryutil
|── serviceaccountutil

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Refactor

Checklist

Documentation added for the feature
CI and all relevant tests are passing
Code Review completed
Verified independently by reviewer

craicoverflow

Calling all of these packages a generic sdk makes things less readable.

craicoverflow · 2022-01-07T11:55:04Z

pkg/cmd/cluster/bind/bind.go

+	"github.com/redhat-developer/app-services-cli/pkg/cmd/cluster/sdk"
+	"github.com/redhat-developer/app-services-cli/pkg/cmd/cluster/sdk/kubeclient"
+	"github.com/redhat-developer/app-services-cli/pkg/cmd/cluster/sdk/services/resources"
+	"github.com/redhat-developer/app-services-cli/pkg/cmd/cluster/sdk/v1alpha"


I would not class the cluster packages as utils - isn't the purpose of this PR to only move utils?

https://breadcrumbscollector.tech/stop-naming-your-python-modules-utils/#:~:text=Why%20utils%20is%20a%20terrible,as%20well%20contain%20almost%20anything.

:)

Yeah I agree. sdk and util is the same thing to me, different names.. cluster should be the package name here.

craicoverflow · 2022-01-07T11:58:52Z

pkg/cmd/cluster/sdk/bind.go

@@ -1,12 +1,12 @@
-package cluster
+package sdk


This package name is too generic IMO. You will have multiple packages called sdk.

https://rakyll.org/style-packages/

craicoverflow · 2022-01-07T12:02:39Z

pkg/cmd/kafka/acl/admin/admin.go

@@ -83,7 +83,7 @@ func NewAdminACLCommand(f *factory.Factory) *cobra.Command {
 			}

 			// user and service account should not allow wildcard
-			if userID == aclutil.Wildcard || serviceAccount == aclutil.Wildcard || userID == aclutil.AllAlias || serviceAccount == aclutil.AllAlias {
+			if userID == sdk.Wildcard || serviceAccount == sdk.Wildcard || userID == sdk.AllAlias || serviceAccount == sdk.AllAlias {


If you look at this line, it is very difficult to understand what sdk relates to. aclutil is much more self descriptive.

<thing>util is a common pattern in Go: https://pkg.go.dev/io/ioutil

I have followed <thing>util for utility methods at top level, i.e pkg/kafkautil, pkg/serviceregistryutil. I was hoping to follow a different naming convention for utils specific to commands, maybe aclcmdutil, topiccmdutil could be better alternative? Wdyt?

I think it is all about import path.

if util/sdk/common has subpackages then we have nice pattern - single name used across all cli packages with specific sub packages.

It might be overkill so them we sacrifice pattern of using single name towards more packages in root - which is ok

Quick look at other projects confirms what enda said. Havents seen packages with common subpackages being used in go

Although our case is different as people might want import sdk etc. so it is self documenting change. etc

Yeah, the import path does not matter a bit, what is important is the name of the package which will be used in the code.

I have followed <thing>util for utility methods at top level, i.e pkg/kafkautil, pkg/serviceregistryutil. I was hoping to follow a different naming convention for utils specific to commands, maybe aclcmdutil, topiccmdutil could be better alternative? Wdyt?

Yes I like that!.

wtrocki · 2022-01-07T12:11:09Z

Reviewed it later than Enda. Fully agree with the naming scheme that Enda proposed.
I'm not 100% sure what to do with cluster logic right now until we have shared service package where we put code that needs to be aware of the all services etc. That is why I think cluster might stay where it is for now? WDYT?

rkpattnaik780 · 2022-01-07T13:10:36Z

That is why I think cluster might stay where it is for now? WDYT?

I will agree, reverting the commit.

wtrocki · 2022-01-08T16:41:47Z

Amazing work!
I think we could start documenting our approach in the docs explaining approaches etc.

We can do that after PR is merged

craicoverflow

LGTM!

craicoverflow · 2022-01-10T10:16:49Z

pkg/cmd/kafka/acl/delete/delete.go

@@ -162,7 +162,7 @@ func runDelete(instanceID string, opts *aclutil.CrudOptions) error {
 		defer httpRes.Body.Close()
 	}

-	err = aclutil.ValidateAPIError(httpRes, opts.Localizer, err, "delete", kafkaInstance.GetName())
+	err = aclcmdutil.ValidateAPIError(httpRes, opts.Localizer, err, "delete", kafkaInstance.GetName())


I think that ValidateAPIError belongs more in an SDK instead of a cmd util, but we can revisit that when the error handling refactor happens.

craicoverflow · 2022-01-10T10:20:20Z

pkg/cmd/kafka/consumergroup/describe/describe.go

-	activeMembersCount := consumergrouputil.GetActiveConsumersCount(consumers)
-	partitionsWithLagCount := consumergrouputil.GetPartitionsWithLag(consumers)
-	unassignedPartitions := consumergrouputil.GetUnassignedPartitions(consumers)
+	activeMembersCount := groupcmdutil.GetActiveConsumersCount(consumers)


nice and concise now :)

craicoverflow · 2022-01-10T10:26:26Z

pkg/cmd/kafka/topic/create/create.go

@@ -203,7 +202,7 @@ func runCmd(opts *options) error {
 }

 func runInteractivePrompt(opts *options) (err error) {
-	validator := topicutil.Validator{
+	validator := topiccmdutil.Validator{


I'd say eventually, validators wont be in the util package as they are not util methods, again that can happen during the SDK creation.

craicoverflow · 2022-01-10T10:30:53Z

pkg/cmd/kafka/topic/topiccmdutil/validators.go

@@ -1,4 +1,4 @@
-package topicutil
+package topiccmdutil


We should probably start to document all pakcage names and their meaning too, this should happen at the top of the package topiccmdutil declaration, but to prevent duplication can be added in a doc.go file: https://go.dev/blog/godoc

craicoverflow · 2022-01-10T10:50:05Z

Amazing work! I think we could start documenting our approach in the docs explaining approaches etc.

We can do that after PR is merged

https://go.dev/blog/godoc

We could document packages using the available Go tooling. You can define it all in a doc.go file: https://github.com/googleapis/google-cloud-go/blob/main/datastore/doc.go

wtrocki · 2022-01-10T10:56:12Z

@craicoverflow @rkpattnaik780 Since we talking - what is your preference:

A) Package based docs like: https://github.com/redhat-developer/app-services-cli/blob/main/pkg/cluster/cluster.go or
https://github.com/googleapis/google-cloud-go/blob/main/datastore/doc.go

B) Markdown format:
https://github.com/kubernetes/client-go/blob/master/INSTALL.md

wtrocki · 2022-01-10T10:59:40Z

Actually realized that syntax for the ones we have is wrong: https://pkg.go.dev/github.com/redhat-developer/[email protected]/pkg/cluster and as enda mentioned we should probably use docs.go instead.
If that is option we can do with it.

refactor: place utility methods in pkg/cmd

c62ff59

openshift-ci bot added the do-not-merge/work-in-progress label Jan 6, 2022

refactor: rename util packages to sdk

379ce33

rkpattnaik780 requested review from craicoverflow and wtrocki January 7, 2022 10:04

craicoverflow suggested changes Jan 7, 2022

View reviewed changes

rkpattnaik780 force-pushed the shared_refactor branch from 21e5ced to 379ce33 Compare January 7, 2022 13:58

fix: rename sdk to cmd specific

c327ea6

rkpattnaik780 requested a review from craicoverflow January 10, 2022 07:51

craicoverflow approved these changes Jan 10, 2022

View reviewed changes

craicoverflow linked an issue Jan 10, 2022 that may be closed by this pull request

Refactor the pkg public API to improve organization. #965

Closed

wtrocki merged commit cc5417f into redhat-developer:development Jan 10, 2022

wtrocki pushed a commit that referenced this pull request Jan 10, 2022

fix: refactor: place utility methods in pkg/cmd (#1371)

76c0a72

wtrocki pushed a commit that referenced this pull request Jan 10, 2022

fix: refactor: place utility methods in pkg/cmd (#1371)

16e2ed3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: refactor: place utility methods in pkg/cmd #1371

WIP: refactor: place utility methods in pkg/cmd #1371

rkpattnaik780 commented Jan 6, 2022 •

edited

Loading

craicoverflow left a comment

craicoverflow Jan 7, 2022

wtrocki Jan 7, 2022

craicoverflow Jan 7, 2022

craicoverflow Jan 7, 2022

craicoverflow Jan 7, 2022

rkpattnaik780 Jan 7, 2022

wtrocki Jan 7, 2022 •

edited

Loading

wtrocki Jan 7, 2022

wtrocki Jan 7, 2022

craicoverflow Jan 7, 2022

craicoverflow Jan 7, 2022

wtrocki commented Jan 7, 2022

rkpattnaik780 commented Jan 7, 2022

wtrocki commented Jan 8, 2022 •

edited

Loading

craicoverflow left a comment

craicoverflow Jan 10, 2022

craicoverflow Jan 10, 2022

craicoverflow Jan 10, 2022

craicoverflow Jan 10, 2022

craicoverflow commented Jan 10, 2022

wtrocki commented Jan 10, 2022

wtrocki commented Jan 10, 2022 •

edited

Loading

WIP: refactor: place utility methods in pkg/cmd #1371

WIP: refactor: place utility methods in pkg/cmd #1371

Conversation

rkpattnaik780 commented Jan 6, 2022 • edited Loading

Directory Structure

Type of change

Checklist

craicoverflow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Jan 7, 2022

rkpattnaik780 commented Jan 7, 2022

wtrocki commented Jan 8, 2022 • edited Loading

craicoverflow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craicoverflow commented Jan 10, 2022

wtrocki commented Jan 10, 2022

wtrocki commented Jan 10, 2022 • edited Loading

rkpattnaik780 commented Jan 6, 2022 •

edited

Loading

wtrocki Jan 7, 2022 •

edited

Loading

wtrocki commented Jan 8, 2022 •

edited

Loading

wtrocki commented Jan 10, 2022 •

edited

Loading