Skip to content
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

replace CommandContext param with RestartOptions type #14

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

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 [email protected]

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 <[email protected]>
@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)

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