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

Deprecate --export flag from get command #73787

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Feb 6, 2019

What type of PR is this?
/kind api-change
/sig cli
/sig architecture
/priority important-longterm

What this PR does / why we need it:
kubectl get --export was broken almost from its inception, multiple bugs (eg. #28551) were reported wrt its functionality but not much was addressed.
Additionally, server-side export (#24855) was brought for discussion and most of the time sig-apimachinery decided not to do it.
Given all of the above I think it's the time to start the process of deprecating this functionality.

Special notes for your reviewer:
/assign @deads2k @liggitt
/cc @kubernetes/api-approvers

Does this PR introduce a user-facing change?:

Deprecate --export flag from kubectl get command.

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. release-note Denotes a PR that will be considered when it comes time to generate release notes. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 6, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl labels Feb 6, 2019
@juanvallejo
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2019
@soltysh
Copy link
Contributor Author

soltysh commented Feb 6, 2019

/hold
I'd like to hear from others too, before we merge this

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2019
@liggitt
Copy link
Member

liggitt commented Feb 6, 2019

I'm in favor of deprecation for the reasons stated, will leave open for comment

@liggitt
Copy link
Member

liggitt commented Feb 6, 2019

cc @kubernetes/sig-api-machinery-misc

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 6, 2019
@bgrant0607
Copy link
Member

I am in favor also.

We need to totally rethink how to support this use case.

@sebgoa
Copy link
Contributor

sebgoa commented Feb 7, 2019

This is a super useful/handy command to help folks move from imperative to declarative.

The nodeport issue is not that problematic and the other issue mentioned dates back April 2016.

I am not in favor, fwiw.

@prabhakar-pal
Copy link

This is a useful feature, is it possible to consider fixing the bugs?

@liggitt
Copy link
Member

liggitt commented Feb 7, 2019

This is a useful feature, is it possible to consider fixing the bugs?

The bugs are secondary. The primary issue is that there are multiple conflicting goals and use cases for this feature that require opposite behavior. Some require dropping cluster-specific info, some require preserving it.

I don't think the current approach to export is consistent or general enough to promote.

@sebgoa
Copy link
Contributor

sebgoa commented Feb 7, 2019

Before deprecating could we have a documented alternative.

End users are already facing the deprecation of kubectl run as a wrapper to create a deployment before kubectl create deployment is ready. (different story I am aware but another CLI deprecation).

Any object can certainly be cleaned up by hand without too much hassle but --export was a quick way to get that done.

Since the issues mentioned as justification go back 2016, might as well solve it first once and for all before deprecating this.

my 2cts.

@soltysh
Copy link
Contributor Author

soltysh commented Feb 7, 2019

Before deprecating could we have a documented alternative.

Like mentioned in the email kubectl get -oyaml is the alternative. You can post-process the resource as you wish.

@sebgoa
Copy link
Contributor

sebgoa commented Feb 7, 2019

Like mentioned in the email kubectl get -oyaml is the alternative. You can post-process the resource as you wish.

I read the email, thank you.

@soltysh
Copy link
Contributor Author

soltysh commented Feb 14, 2019

I think this had sufficient soak time.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2019
@jbrownsc
Copy link

Not having to do this would be great! https://medium.com/@jonathan.johnson/export-has-been-deprecated-in-1-14-51cfef5a0cb7

@srl295
Copy link

srl295 commented Oct 29, 2019

Please provide an alternative in the deprecation message.

Or, how about:

  • get … --omit-namespace --namespace=source (just drop namespace from output)
    and/or
  • apply … --ignore-namespace --namespace=target (ignore namespace in yaml)

@Jean-Mercier
Copy link

we use export command to manage compliance check between our CI and clusters

how can we do without this command ?

@mvdan
Copy link
Contributor

mvdan commented Nov 22, 2019

I needed something like this at work yesterday, so I threw together some filters with https://github.com/kislyuk/yq. In case they are useful to others:

$ cat filter.jq
del(.. | select(. == "" or . == null or . == "None")) |
walk(if type == "object" then del(.status,.annotations,.creationTimestamp,.generation,.selfLink,.uid,.resourceVersion) else . end) |
del(.. | select(. == {}))
$ kubectl get -o=yaml | yq --yaml-output "$(cat filter.jq)"

This only covers the basics, but the timestamps/status/annotations and zero-valued fields were what caused most of the pain for us.

@Jean-Mercier
Copy link

Jean-Mercier commented Nov 22, 2019

I needed something like this at work yesterday, so I threw together some filters with https://github.com/kislyuk/yq. In case they are useful to others:

$ cat filter.jq
del(.. | select(. == "" or . == null or . == "None")) |
walk(if type == "object" then del(.status,.annotations,.creationTimestamp,.generation,.selfLink,.uid,.resourceVersion) else . end) |
del(.. | select(. == {}))
$ kubectl get -o=yaml | yq --yaml-output "$(cat filter.jq)"

This only covers the basics, but the timestamps/status/annotations and zero-valued fields were what caused most of the pain for us.

Thanks

I try it but i have this error message

kubectl get all -o=yaml | yq --yaml-output "$(cat filter.jq)" jq: error: walk/1 is not defined at <top-level>, line 2: walk(if type == "object" then del(.status,.annotations,.creationTimestamp,.generation,.selfLink,.uid,.resourceVersion) else . end) | jq: 1 compile error

Edit thanks mvdan i fix it with manual install of jq-1.6 ( with yum install the latest was 1.5) and yq v2.9.2
i will play with this

@mvdan
Copy link
Contributor

mvdan commented Nov 22, 2019

yq is a wrapper for jq. Make sure that the versions of both are new enough. It works here, so I don't know. It's probably best that we don't debug the hack in this thread, because we'll annoy the 30+ people watching the thread.

@sheerun
Copy link

sheerun commented Dec 3, 2019

So now I'm supposed to copy secret in postintall hook with hypercube I need to either use weird tricks with sed, install jq or install whole kubernetes plugin when it was as simple as --export? Wow... This feature was deprecated after one week of non-discussion, and now has almost 100 upvtes to de-depreacte it, could you consider it?

@relgames
Copy link

relgames commented Dec 19, 2019

Came here after getting a deprecation warning. Motivation behind removal is questionable. "We can't fix bugs so we just remove the feature". Well.

Also:

This pull-request has been approved by: soltysh

As a software developer, I consider approving own PRs a dangerous practice.

@liggitt
Copy link
Member

liggitt commented Dec 19, 2019

This feature was deprecated after one week of non-discussion, and now has almost 100 upvtes to de-depreacte it, could you consider it?

Not in its current form. This was the visible outcome of more than a year of bug reports, discussion about the structure of the current implementation, and the deprecation of the corresponding server-side API parameters, e.g.:

Motivation behind removal is questionable. "We can't fix bugs so we just remove the feature".

The rationale was explained in #73787 (comment)

As a software developer, I consider approving own PRs a dangerous practice.

That is auto-recorded by the PR bots (authors inherently approve their own PRs). The author did not rely on that approval, but held and requested additional feedback, and got approval from two top-level approvers(1, 2).

A proposal can be opened to address some of the use cases discussed in this thread, which takes into account the shortcomings of the previous implementation.

@marcellodesales
Copy link

@mvdan @Jean-Mercier

$ kubectl -n istio-system get service istio-ingressgateway -o yaml | yq --yaml-output "$(cat clean-k8s-clone.jq)"
Error: unknown flag: --yaml-output

Instead, I'm using jq to process everything and yq to render the final yaml object from the resulting json...

kubectl -n istio-system get service igateway -o json | jq "$(cat filter.jq)" | yq r -

@lavalamp
Copy link
Member

@apelisse @jennybuckley we should consider a command to filter an object, leaving only a specific manager's fields, and then printing the result. It wouldn't work 100% of the time, but this would let people see more or less what the user intent was in many cases.

@apelisse
Copy link
Member

Yeah, even though I just read the entire thread here (😅) and it's not clear to me what problem we're trying to solve.

we should consider a command to filter an object, leaving only a specific manager's fields, and then printing the result. It wouldn't work 100% of the time, but this would let people see more or less what the user intent was in many cases.

I do believe that this would fix some of the problems though, it seems "easily" doable but it could indeed create configs that are not "apply-able".

I'd love to understand the problem space better, I've read mostly:

  • Config contains the namespace so it needs to be removed so it can be copied in a different namespace,
  • Copy configs from one namespace to another, even those configs that haven't been applied,

Both of these seem to be XY problems rather than actual problems. I'd rather understand what these people are really trying to achieve. Copying objects from one namespace to another is not a recommended practice.

@mltsy
Copy link

mltsy commented Jan 30, 2020

@apelisse - happy to share my "Y" issue :)
Here's an example of where I'm using --export in a deploy script (in Google Cloud Build):

kubectl get secret tls-${_ENV}-secret --namespace=default --export -o yaml |
  kubectl apply -n ${_PROJECT_NAME} -f -

We have a single TLS secret generated for our staging environment (from a single wildcard certificate), stored in the default namespace and want to copy it to each project namespace when a project is deployed, so that the build script doesn't have to handle generating a new secret for every project.

@mltsy
Copy link

mltsy commented Mar 11, 2020

Update, for what it's worth: we ended up using cert-manager to generate all the certificates, which was pretty straight-forward once we got around to it (just adding an annotation to each ingress configuration for cert-manager to find and handle with Lets Encrypt). In our case, this eliminated the need for --export.

@Kyrluckechuck
Copy link

Just a warning/notice that this has been officially removed with v1.19.0 via #88649 for anyone that is investigating why their scripts suddenly stopped working completely.

@dfang
Copy link
Contributor

dfang commented Sep 26, 2020

if you deprecate a feature, can you suggest an alternative in command deprecation messages ?

@mltsy
Copy link

mltsy commented Sep 28, 2020

@dfang do you have a specific use case you'd like a recommendation for? I agree that's a great idea generally, but I can also see how that would be difficult to do in this case, since the use cases for --export are varied, and therefore the alternatives are somewhat unique to each use case. Would have possibly been nice to link to this PR at least for a few suggestions.

(Maybe as a general practice, it would work to create and/or link to a Github issue for each deprecation, as a place to find and discuss alternatives!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet