refactor: refactor the package structure and layout

[craicoverflow]

The purpose of this refactor was to make the general layout more consistent and try to group files and packages by their domains as much as possible. This by no means constitutes a final CLI SDK, but I believe it will make it easier to build the SDK when ready, as much of the structure is already logically grouped together.

There are 506 files in this PR, many of those include file renames, generated content etc which are hidden by default so the real figure to review is closer to ~200. And that is mainly package renames.

ℹ️ Ignore the files in ./pkg/apis/ams as these are generated and do not need to be reviewed.

I have reviewed this and verified a couple of times myself, but needs more eyes.

ℹ️ I have no strong attachment to this PR. If you feel we should wait and do this together with the SDK epic I'm totally fine with that.

pkg
├── accountmgmtutil
# Renamed `api` to `apis` to indicate that this holds some minimal API clients, which do not fit into the RHOAS SDK.
├── apis
│   ├── ams
│   └── rbac
# auth logic is grouped together
├── auth
│   ├── login
│   ├── pkce
│   └── token
├── cmd
│   ├── cluster
│   ├── completion
│   ├── debug
│   ├── docs
│   ├── kafka
│   ├── login
│   ├── logout
│   ├── registry
│   ├── root
│   ├── serviceaccount
│   ├── status
│   ├── version
│   └── whoami
# created a distinct "core" package to logically group common packages together. This may form the basis of the SDK
├── core
│   ├── cluster
│   ├── cmdutil
│   ├── config
│   ├── connection
│   ├── errors
│   ├── httputil #should hold utils relating to HTTP (HTTP logging middleware etc)
│   ├── ioutil # ioutil should hold utils relating to input/output
│   ├── localize
│   ├── logging
│   └── status
# services each get a `<service>util` package
├── kafkautil
│   ├── aclutil
│   ├── consumergrouputil
│   └── topicutil
├── serviceaccountutil
│   ├── credentials
│   └── validation
└── serviceregistryutil

internal/
├── build
├── doc
├── mockutil
├── telemetry

[wtrocki]

@craicoverflow can you transform that PR to document so we can discuss, ask questions and comment on the package structure?

[wtrocki]

I will try to ask questions inline:

├── kafkautil

Why would that be outside core? Any special meaning/plans for that packages?

Core looks good but it is mixed bag of responsibilities. I see some utils but also command line logic like status.
What is the plan for this? Do we keep utils in core + logic. Am I right to say that plan is to have

• cmd (cobra stuff and business logic for simple commands)
• logic as SDK (for reusable logic in the CLI)
• core (utils)

[craicoverflow]

I will try to ask questions inline:

├── kafkautil

Why would that be outside core? Any special meaning/plans for that packages?

Core is designated for packages without any attachment to one specific section of the SDK or CLI. Things like logging, i18n and config are general purpose which touch every area of the CLI.
My question is why do you think kafkautil should be in core?

I see some utils but also command line logic like status

This is not the status command, it is the library which does generate the service status metadata for the status command.

Core looks good but it is mixed bag of responsibilities.

Other than status (which I think I clarified is not the command) can you be explicit about what you think is mixed?

Do we keep utils in core + logic

What is the utils in core you speak of? What is "logic" this is a vague term..it is all logic?

Am I right to say that plan is to have

• cmd (cobra stuff and business logic for simple commands)
• logic as SDK (for reusable logic in the CLI)
• core (utils)

I'm not sure how you think core os "utils", can you maybe expand on that point.

[craicoverflow]

@wtrocki @rkpattnaik780 changed the base branch to development which we can use over the Christmas break as our base.

[rkpattnaik780]

# services each get a `<service>util` package
├── kafkautil
│   ├── aclutil
│   ├── consumergrouputil
│   └── topicutil
├── serviceaccountutil
│   ├── credentials
│   └── validation
└── serviceregistryutil

Rather than having the utility methods outside, can we have them in cmd instead, with a shared folder for each group of subcommands?

[craicoverflow]

Rather than having the utility methods outside, can we have them in cmd instead, with a shared folder for each group of subcommands?

Could you mock that up in the same way I have done so I can visualise it? I'm trying to understand the multiple "shared" folder concept and what it is for.

[rkpattnaik780]

Could you mock that up in the same way I have done so I can visualise it?

├── cmd
│   ├── kafka
|           |─── create
|           |─── update
|           |─── topic
|                   |─── create
|                   |─── update
|                   |─── ...
|                   |─── shared
|           |─── ......
|           |─── shared

While this wouldn't be a full replacement to <service>util packages, this could be a good place to keep some util methods.

[craicoverflow]

That could make sense. Would the utils which are under cmd/kafka/shared be only intended for the cmd/kafka commands in that case? If yes I think it makes sense.

By the way, overly broad package names like "shared" and "util" and "common" are not recommended in Go, Probably "core" is not good either though.

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

[rkpattnaik780]

That could make sense. Would the utils which are under cmd/kafka/shared be only intended for the cmd/kafka commands in that case? If yes I think it makes sense.

Yes.

By the way, overly broad package names like "shared" and "util" and "common" are not recommended in Go, Probably "core" is not good either though.

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

I agree. Maybe we could use prefix for naming the shared folders

[craicoverflow]

@rkpattnaik780 what should we do with this PR?

[wtrocki]

As agreed lets:

• rebase changes
• do not rename ams and other not essential renames to reduce number of files changed

[craicoverflow]

Updated. Currently at 222 files changed.

cc @wtrocki @rkpattnaik780

[wtrocki]

Do you mind excluding those changes as well.

There is #1352 that addresses that

[wtrocki]

Do we know why this appears as new file?

[craicoverflow]

Because it is a new file. Create package separation between interface and implementation to prevent circular dependency. Maybe there is a better way.

[wtrocki]

Reviewed and verified!

[rkpattnaik780]

@craicoverflow the contents of core/cluster seem a bit of deviation from other contents of core, since they are mostly linked to the cluster commands, do you think something like <service>util could be better?

[craicoverflow]

@craicoverflow the contents of core/cluster seem a bit of deviation from other contents of core, since they are mostly linked to the cluster commands, do you think something like <service>util could be better?

Yes, I can change that tomorrow.

[wtrocki]

Merging - we can apply other fixes separately - like moving status to separate package or moving cluster commands