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

cmd: flag parsing cleanup #94

Merged
merged 1 commit into from Oct 14, 2018

Conversation

Projects
None yet
4 participants
@ahmetb
Copy link
Contributor

commented Oct 12, 2018

  • simplify flag parsing logic (fixes #83)
  • hide all glog files but -v (regardless of running as plugin or not)
  • call GetExecutedVersion only in "version" cmd
    • TODO: maybe remove GetExecutedVersion, not used in any decisions
  • unvendor viper (and many of its dependencies) since unused.
@codecov-io

This comment has been minimized.

Copy link

commented Oct 12, 2018

Codecov Report

Merging #94 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #94   +/-   ##
=======================================
  Coverage   47.32%   47.32%           
=======================================
  Files          12       12           
  Lines         598      598           
=======================================
  Hits          283      283           
  Misses        268      268           
  Partials       47       47

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4b6e0...6e52dd9. Read the comment docs.

@ahmetb ahmetb requested a review from lbb Oct 12, 2018

@lbb

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

needs rebase

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

rebased

@ahmetb ahmetb force-pushed the ahmetb:flags-cleanup branch from 38d2ef2 to 6707e8d Oct 12, 2018

Show resolved Hide resolved cmd/krew/cmd/version.go

@ahmetb ahmetb force-pushed the ahmetb:flags-cleanup branch from 6707e8d to a4e7732 Oct 13, 2018

@@ -53,14 +54,18 @@ kubectl plugin upgrade foo bar"`,
pluginNames = args
}

executedKrewVersion, _, err := environment.GetExecutedVersion(paths.InstallPath(), os.Args[0], environment.Realpath)

This comment has been minimized.

Copy link
@lbb

lbb Oct 14, 2018

Contributor

args[0] is not the absolute binary location. We need os.Executeable()

This comment has been minimized.

Copy link
@ahmetb

ahmetb Oct 14, 2018

Author Contributor

args[0] is not the absolute binary

This worked fine when I manually tested. But I'll fix.

@lbb

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

LGTM when fixed

@ahmetb ahmetb force-pushed the ahmetb:flags-cleanup branch from a4e7732 to 77d979f Oct 14, 2018

cmd: flag parsing cleanup
- simplify flag parsing logic (fixes #83)
- hide all glog files but -v (regardless of running as plugin or not)
- call GetExecutedVersion only in "version" cmd
  - TODO: maybe remove GetExecutedVersion, not used in any decisions
- unvendor viper (and many of its dependencies) since unused.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

@ahmetb ahmetb force-pushed the ahmetb:flags-cleanup branch from 77d979f to 6e52dd9 Oct 14, 2018

@ahmetb ahmetb merged commit cde8c63 into kubernetes-sigs:master Oct 14, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.