Skip to content

Conversation

@xcoulon
Copy link
Contributor

@xcoulon xcoulon commented Mar 19, 2024

inspired by the GetOptions type in the kubectl get command,
mostly because CommandContext is confusing (I know, I contributed to it!)

Signed-off-by: Xavier Coulon xcoulon@redhat.com

inspired by the 'GetOptions' type in the [kubectl get command](https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/get/get.go#L56-L86),
mostly because `CommandContext` is confusing (I know, I contributed to it!)

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@xcoulon
Copy link
Contributor Author

xcoulon commented Mar 19, 2024

This is a draft to propose the change and discuss about it. If we agree on the design, I can apply the same refactoring to all the commands so we can get rid of the CommandContext (whose intent was to encapsulate multiple objects, but whose name remains confusing IMO)

@codecov
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 68.42105% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 65.35%. Comparing base (dd44578) to head (35a6fba).

Files Patch % Lines
pkg/cmd/adm/restart.go 68.00% 7 Missing and 1 partial ⚠️
pkg/cmd/adm/register_member.go 66.66% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master      #14   +/-   ##
=======================================
  Coverage   65.35%   65.35%           
=======================================
  Files          38       38           
  Lines        1980     1980           
=======================================
  Hits         1294     1294           
  Misses        528      528           
  Partials      158      158           
Flag Coverage Δ
unittests 65.35% <68.42%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metlos
Copy link
Contributor

metlos commented May 3, 2024

As a newcomer to the codebase I possibly don't see all the problems that have been encountered by I do like the idea of the CommandContext and don't find it particularly confusing.

What I do find is that in addition to the context and terminal, some commands require additional parameters. I my refactoring of the register-member command I'm not even using the CommandContext.NewClient anymore and instead am using another way of instantiating the client.

So IMHO, the commands should still make use of the CommandContext to pass the context and terminal around but also could theoretically require command-specific *Options as you suggest in this PR. This way, the command context would only contain stuff common to all commands and options could encapsulate the command-specific additional requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants