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

Print kubectl client version #991

Merged
merged 1 commit into from
Sep 21, 2018

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Sep 18, 2018

Signed-off-by: David Gageot david@gageot.net

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from the linter failing, one nit otherwise LGTM

@@ -64,6 +64,8 @@ func (k *KubectlDeployer) Labels() map[string]string {
// Deploy templates the provided manifests with a simple `find and replace` and
// runs `kubectl apply` on those manifests
func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]Artifact, error) {
logrus.Debugln("kubectl client version:", k.kubectl.Version())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would even consider making this Info level log

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

@dgageot dgageot force-pushed the print-kubectl-version branch from 9d6da67 to ab1ee87 Compare September 20, 2018 06:48
@codecov-io
Copy link

codecov-io commented Sep 20, 2018

Codecov Report

Merging #991 into master will decrease coverage by 0.25%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #991      +/-   ##
==========================================
- Coverage   40.17%   39.92%   -0.26%     
==========================================
  Files          68       69       +1     
  Lines        3004     3026      +22     
==========================================
+ Hits         1207     1208       +1     
- Misses       1671     1692      +21     
  Partials      126      126
Impacted Files Coverage Δ
pkg/skaffold/deploy/kubectl/cli.go 0% <ø> (ø) ⬆️
pkg/skaffold/deploy/kubectl/version.go 0% <0%> (ø)
pkg/skaffold/deploy/kubectl.go 51.21% <100%> (+0.6%) ⬆️

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 1f1ec2d...3158437. Read the comment docs.

@dgageot
Copy link
Contributor Author

dgageot commented Sep 21, 2018

@nkubala It should be all good now.


buf, err := c.getVersion(context.Background())
if err != nil {
logrus.Errorln("unable to get kubectl client version", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a warning instead of an error since it's not really fatal?

Signed-off-by: David Gageot <david@gageot.net>
@dgageot dgageot force-pushed the print-kubectl-version branch from ab1ee87 to 3158437 Compare September 21, 2018 14:01
@dgageot dgageot merged commit 7755b82 into GoogleContainerTools:master Sep 21, 2018
@dgageot dgageot deleted the print-kubectl-version branch December 28, 2018 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants