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

Bump Golang 1.12.3 #1818

Merged
merged 1 commit into from
Apr 12, 2019
Merged

Conversation

tao12345666333
Copy link
Contributor

Signed-off-by: Jintao Zhang zhangjintao9020@gmail.com

- What I did

go1.12.3 (released 2019/04/08) fixes an issue where using the prebuilt binary releases on older versions of GNU/Linux led to failures when linking programs that used cgo. golang/go#31293

see: https://github.com/golang/go/issues?utf8=✓&q=milestone%3AGo1.12.3

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
- ps: .\scripts\make.ps1 -TestUnit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is because of no newline at end of file.

@@ -1,4 +1,4 @@
FROM dockercore/golang-cross:1.12.2@sha256:ea93d7ed5b464e5163cf8df40a198ad54afe6a59e1ca335c9bc4a5ed3702f2d0
FROM dockercore/golang-cross:1.12.3@sha256:b88a6be2469d321601f87675c49d1b3f166c4c37abc5ce3e74557fc6660481be
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as @thaJeztah said,

I'm actually thinking of removing this "pin by digest", as it doesn't add much for our use-case 🤔

I also agree very much. Do we need to remove this "pin by digest" ? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I am ok with removing it

@vdemeester @silvin-lubecki SGTY?

@tao12345666333
Copy link
Contributor Author

It seems that the lint task failed, I ran the lint task locally and found that it takes 4m+.

/go/src/github.com/docker/cli # time /usr/local/bin/gometalinter --config=gometalinter.json ./...
real    4m 55.06s
user    1m 55.14s
sys     0m 26.43s

should we increasing deadline? 🤔

@codecov-io
Copy link

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1818   +/-   ##
=======================================
  Coverage   56.31%   56.31%           
=======================================
  Files         308      308           
  Lines       21429    21429           
=======================================
  Hits        12068    12068           
  Misses       8476     8476           
  Partials      885      885

@tao12345666333
Copy link
Contributor Author

tao12345666333 commented Apr 11, 2019

And I found gometalinter has been archived by the owner. alecthomas/gometalinter#590

should we change to use golangci-lint or other active lint tool?

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks!

Left one question about the timeout, and let's wait for the others to reply wrt the digest

gometalinter.json Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
FROM dockercore/golang-cross:1.12.2@sha256:ea93d7ed5b464e5163cf8df40a198ad54afe6a59e1ca335c9bc4a5ed3702f2d0
FROM dockercore/golang-cross:1.12.3@sha256:b88a6be2469d321601f87675c49d1b3f166c4c37abc5ce3e74557fc6660481be
Copy link
Member

Choose a reason for hiding this comment

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

I am ok with removing it

@vdemeester @silvin-lubecki SGTY?

@thaJeztah
Copy link
Member

For the linter: I just recalled there's a pull request to replace it (that I didn't review yet 🤭🤫😳) #1797

@thaJeztah
Copy link
Member

Interesting;

cli/command/formatter/reflect_test.go:15:34:warning: nolint directive did not match any issue (nolint)
WARNING: deadline exceeded by linter unused (try increasing --deadline)
Exited with code 3

@thaJeztah
Copy link
Member

( nolint directive did not match any issue (nolint))

@tao12345666333
Copy link
Contributor Author

For the linter: I just recalled there's a pull request to replace it (that I didn't review yet 🤭🤫) #1797

Sorry, I didn't notice its existence. I just tried to change gometalinter to golangci-lint locally. 😆

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, all green now 👍

ping @vdemeester @silvin-lubecki

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@vdemeester vdemeester merged commit 893f4a1 into docker:master Apr 12, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Apr 12, 2019
@tao12345666333 tao12345666333 deleted the bump-golang-1.12.3 branch April 12, 2019 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants