add a flag for the path in export logs #1519
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder 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 |
k8s-ci-robot
added
approved
| } else { | ||
| logger.Warn("WARNING: --loglevel is deprecated, please switch to -v and -q!") | ||
| } | ||
| cmd.FancyWarn(logger, "--loglevel is deprecated, please switch to -v and -q!") |
There was a problem hiding this comment.
not related, just DRYing this out.
|
there is also I'm going to tweak this slightly, but also taking a stronger look at what the ecosystem is doing these days. |
k8s-ci-robot
added
the
do-not-merge/hold
|
I do think we should rethink this tool a bit regardless though, the tempdir thing is a bit strange in retrospect. |
BenTheElder
force-pushed
the
flag-out
branch
3 times, most recently
from
eeefe39 to
cc6a57b
Compare
|
Updated:
Behavior:
Should be non-breaking for now (though warnings) with an easy path to switch your arg usage to a flag. This helps free up options in the future for arguments and brings behavior more in line with other archiving / building tools. Users of the tempdir mode are probably quite rare, we could add a special alternative flag for them if we find someone unable of using something like It is not optimal to dump to a tempdir generally, you will wind up having to copy all of it to somewhere useful. Not 100% convinced of this change though. Leaving the hold for now for others to inspect. Need to think more about how we handle other commands and try to find more usage in the wild. |
|
I never commented here, but |
|
I think we want to add a new the issue is, do we take this opportunity to move to a more normal kubernetes UX with we could retrofit what about xref: #1766 |
do you think
|
|
i'm not sure honestly, but I think I'd still prefer to move this (directory) to a flag. |
|
/retest |
|
@BenTheElder: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
kind export logsis currently the only CLI command accepting an argument, and it only accepts this one argument. All other commands require zero arguments.This PR creates a migration path off of that argument by adding a flag to control it instead.
We can explore supporting arguments for
--namein v0.9.0, which is more intuitive / ecosystem compatible in retrospect.minikube's--profilewas probably not desirable to mimic.i.e. we can enable
kind get cluster kind2<->kubectl get po bazin a PR after v0.8.0 is out (so v0.8.0 only warns about the argument but still supports it and provides time to migrate).