-
Notifications
You must be signed in to change notification settings - Fork 16
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
global: re-organize package structure #83
Conversation
2a55ec2
to
26053d3
Compare
26053d3
to
4a06d6e
Compare
7153736
to
72fd6d2
Compare
72fd6d2
to
162f50f
Compare
pkg/workflows/workflows.go
Outdated
*/ | ||
|
||
// Package workflows gives utility functions related to REANA's workflows. | ||
package workflows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package name workflows
seems "important", but its contents seems "underused". Might be better called workflowutils
, but that's weird. We can keep it as it, and think whether it wouldn't grow in importance, say workflows/list.go
, workflows/status.go
, etc to hold more of the "core" business logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're planning to extend the package that way, should we at least rename the file to utils.go
, without changing the package name?
config/config.go
Outdated
*/ | ||
|
||
// Package config gives constants and small functions that specify the REANA client configuration. | ||
package config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have three package levels in this PR: (i) cmd
with cobra stuff, (ii) config
for abstracting hard-coded values, and (iii) pkg
where all the displaying, formatting etc utility live (in their respective packages). The fact that a lot of business logic lives under cmd
and not pkg
creates a certain dissonance.
It may be good to find a way to have cmd
stored under pkg
, as other software projects do (e.g. kubectl, kind). In this way it'll be clear where the most important stuff is. Also, config
should then be moved there.
Note also that kubectl, kind and friends usually not use single cmd
package for everything, but introduce packages for each command, which in our case would be list
, status
, etc. This would make sense because the whole reana-client-go is about CLI commands, so keeping cobra layer tiny, just for parsing CLI options etc, whilst having clearly visible business logic packages implementing each command business logic, would makes sense. From the pure package naming point of view, it seems advantageous to keep "cmd" tiny and place focus rather on the business logic itself.
Something we can think for later as the functionality grows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved config
inside pkg
. Separating the business logic from the cobra template seems out of reach of this PR but I could try creating different packages for each command. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can go like this, and think about possible further changes as we shall be adding upload/secret/etc functionality.
162f50f
to
4f02609
Compare
4f02609
to
ac2be4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely, and looks better 👍 And we can consider some further changes to the "core" business logic, such as using single cmd
package for all commands, vs using separate packages for separate commands or command groups, later on.
closes #80
review after #82