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

Umbrella Issue: refactor commands to split flags from options #1046

Open
soltysh opened this issue Apr 28, 2021 · 36 comments
Open

Umbrella Issue: refactor commands to split flags from options #1046

soltysh opened this issue Apr 28, 2021 · 36 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@soltysh
Copy link
Contributor

soltysh commented Apr 28, 2021

Currently most or even all of the commands have an *Options structure holding all of the data needed to run a command and three main methods Complete (finishing *Options struct), Validate (validating *Options struct) and Run (actual code behind the command). The current problem is that these *Options struct is tightly coupled with flags, but we'd like to split these.

A perfect example is wait command.

This issue should serve as a synchronization point.

/cc @eddiezane @seans3

@soltysh soltysh added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/design Categorizes issue or PR as related to design. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Apr 28, 2021
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 28, 2021
@soltysh
Copy link
Contributor Author

soltysh commented Apr 28, 2021

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 28, 2021
@eddiezane
Copy link
Member

/remove-help
/remote-good-first-issue

Holding off on this until we scope better and trial a few to confirm it's what we want.

@k8s-ci-robot k8s-ci-robot removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Apr 28, 2021
@eddiezane
Copy link
Member

eddiezane commented Apr 28, 2021

@soltysh looking at logs and get

@eddiezane
Copy link
Member

@rikatz will take basic create

@eddiezane
Copy link
Member

@KnVerey to look at drain or describe

@eddiezane
Copy link
Member

@eddiezane will look at a simple config command

@rikatz
Copy link
Contributor

rikatz commented Apr 28, 2021

/assign
So I wont forget :)

@lauchokyip
Copy link
Member

lauchokyip commented Apr 30, 2021

Will do create with @rikatz synchronously

@SaiHarshaK
Copy link
Contributor

SaiHarshaK commented May 1, 2021

/assign
I'll try to do for apply command

@ihcsim
Copy link

ihcsim commented May 4, 2021

/assign

@ihcsim
Copy link

ihcsim commented May 4, 2021

i can do attach.

@pacoxu
Copy link
Member

pacoxu commented May 7, 2021

A simple summary:

@eddiezane
Copy link
Member

@BigaDev will take a look at scale

@eddiezane
Copy link
Member

@pacoxu thanks for the summary!

I want to make sure folks understand that this will be a bit of an iteration process while we dial in exactly what we want these to look like. Expect some refactors and potential rewrites.

@PuneetPunamiya
Copy link
Contributor

I'll refactor explain :)

@varksvader
Copy link

Hey! I was wondering if you guys needed more help on this issue. I'm happy to pick up a command and start working on it.

@varksvader
Copy link

/assign

Talked to owner of issue. Grabbing drain to refactor.

@harshitasao
Copy link

I'll refactor create
/assign

@lauchokyip
Copy link
Member

@harshitasao , I think I finished create a while ago (kubernetes/kubernetes#101736), just waiting for review, would you be able to pick other command to refactor instead?

@k8s-ci-robot
Copy link
Contributor

@brunopecampos: GitHub didn't allow me to assign the following users: brunopecampos.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign

I'll do diff

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jaehnri
Copy link
Member

jaehnri commented Jan 18, 2023

/unassign

We hit the 10 assignees limit. Unassigning so Bruno can do diff :)

@brunopecampos
Copy link

/assign

I'll do diff

@varshaprasad96
Copy link
Contributor

I can work on the refactoring of expose command.

@gxwilkerson33
Copy link
Contributor

@eddiezane @soltysh Do we have a list of commands that still need this refactoring?

@gxwilkerson33
Copy link
Contributor

@soltysh @eddiezane I am working on refactoring of Run command. Currently id does not follow Complete, Validate, Run. Validate seems to be being handled within the Run function. Should i make that refactor also, to make Validate function and remove this functionality from the Run function?

@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging a pull request may close this issue.