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

Remove cmd from kubectl/cmd/factory arguments #4470

Closed
smarterclayton opened this issue Feb 17, 2015 · 5 comments
Closed

Remove cmd from kubectl/cmd/factory arguments #4470

smarterclayton opened this issue Feb 17, 2015 · 5 comments
Labels
area/kubectl kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@smarterclayton
Copy link
Contributor

@jlowdermilk I think the need for this argument to the factory methods has passed - it means that callers who want to provide configurable behavior have to do it through the factory itself, which is acceptable.

We can remove it whenever.

@smarterclayton smarterclayton added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Feb 17, 2015
@davidopp davidopp added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 17, 2015
@goltermann goltermann added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Feb 18, 2015
@tmrts
Copy link
Contributor

tmrts commented Mar 14, 2015

I removed most of the unused arguments. But, There are a few methods still using the cmd argument. Namely, factory.Validator, PrintObject, PrinterForMapping and ClientMapperForCommand. Before proceeding any further I wanted to ask what should be done about these methods? Deprecated/Refactored/Removed ?

@smarterclayton
Copy link
Contributor Author

The factory methods were the most important. The pattern before was people using the flags by name (cmd.Flags().Get("foo")), but now factory methods must use pointer flag vars, so there's nothing that should need the command there.

The printer methods are optional units that can be used when you add the flags for them to your command. Unless we refactor the definition of those flags to use pointers and return a printer struct (something I had started a while back and never finished), it's not worth changing them right now.

The other consumer of factory is OpenShift - we wrap the factory to add our additional types (implementations of things like describers, action verbs, etc): https://github.com/openshift/origin/blob/master/pkg/cmd/util/clientcmd/factory.go - but we will pick up and remove use of cmd when we rebase.

On Mar 14, 2015, at 6:53 AM, Tamer TAS notifications@github.com wrote:

I removed most of the unused arguments. But, There are a few methods still using the cmd argument. Namely, factory.Validator, PrintObject, PrinterForMapping and ClientMapperForCommand. Before proceeding any further I wanted to ask what should be done about these methods? Deprecated/Refactored/Removed ?


Reply to this email directly or view it on GitHub.

@tmrts
Copy link
Contributor

tmrts commented Mar 15, 2015

Thanks for the explanation. I've cleaned the factory.Validation as well. Print method APIs are untouched as you suggested.

you-n-g added a commit to you-n-g/kubernetes that referenced this issue Mar 29, 2015
related issue "Remove cmd from kubectl/cmd/factory arguments kubernetes#4470"
akram pushed a commit to akram/kubernetes that referenced this issue Apr 7, 2015
related issue "Remove cmd from kubectl/cmd/factory arguments kubernetes#4470"
@bgrant0607
Copy link
Member

cc @krousey

@bgrant0607
Copy link
Member

Looks like this was done by PR #6144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

5 participants