Global flag refactor #792
Global flag refactor #792
Conversation
|
/retest |
|
looks like it fails on ci.. but integration-tests work fine locally... I'll dig in tomorrow. |
| @@ -34,16 +34,14 @@ var ( | |||
| ) | |||
|
|
|||
| type options struct { | |||
| install.RepositoryOptions | |||
There was a problem hiding this comment.
Nice usage of embedded types. Out of curiosity: what is your reasoning to embed a struct vs. making it a field e.g. Repo install.RepositoryOptions?
There was a problem hiding this comment.
this example is too simple to see it yet... but the flatter structure is easy to work with (although involves some magic:( ). I was thinking of having grouping of common options (for instance options needed for working with repos, options needed for working with k8s). Then embed those options into the options for your command. I was further thinking of having a function for each of these structs for flag setup... thereby guaranteeing that cli messages are consistent. This would result in outer structure function calling the inner... I'm not sure if it adds too much complexity for what it gains. thoughts welcome 🙏
| @@ -34,16 +34,14 @@ var ( | |||
| ) | |||
|
|
|||
| type options struct { | |||
| install.RepositoryOptions | |||
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gerred, kensipe, zen-dog The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
kensipe
force-pushed
the
ken/global-env
branch
from
6f1457e
to
ea12317
Compare
What type of PR is this?
/component kudoctl
/kind cleanup
What this PR does / why we need it:
It was decided that we want to remove
--repoout of globals@gerred and I discussed adding
--namespaceinto globalsThere is some refactored code to support the changes
Which issue(s) this PR fixes:
Fixes #760
Special notes for your reviewer:
Think about creating a common struct for repo needs and having a function that will add those details to flags.
Does this PR introduce a user-facing change?: