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.

Sign up for GitHub

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

Jump to bottom

chore(*): update golangci-lint version from 1.16.0 to 1.20.1 #14

Merged
merged 1 commit into from
Nov 8, 2019
Merged

chore(*): update golangci-lint version from 1.16.0 to 1.20.1 #14

merged 1 commit into from
Nov 8, 2019

Conversation

zhujian7
Copy link
Contributor

@zhujian7 zhujian7 commented Oct 25, 2019 •
edited
Loading

What this PR does / why we need it:

Golangci-lint 版本升级后能减少大量的内存消耗。即使去掉 GOGC=5 的配置，升级后的版本消耗的 memory 也更小。

  • golang-lint: 1.16.0 , GOGC=5
    image
  • golang-lint: 1.20.1 , GOGC=5
    image
  • golang-lint: 1.20.1 , GOGC default
    image

Which issue(s) this PR is related to (optional, link to 3rd issue(s)):

Fixes #

Reference to #

Special notes for your reviewer:

/cc @bbbmj @caicloud/yilei-write @supereagle

Release note:

NONE

@caicloud-bot caicloud-bot added release-note-none Denotes a PR that doesn't merit a release note. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 25, 2019
supereagle
supereagle reviewed Oct 25, 2019
View reviewed changes
.golangci.yml Outdated
@@ -1,6 +1,6 @@
run:
# concurrency: 2
deadline: 5m
deadline: 10m
Copy link
Member

Choose a reason for hiding this comment

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

Still need to increase deadline?

Copy link
Contributor

Choose a reason for hiding this comment

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

deadline 这个参数已经废弃，使用 timeout 代替 deadline。
golangci/golangci-lint#793
golangci/golangci-lint#822

Copy link

Choose a reason for hiding this comment

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

这个还是要放开各个组件自己可以改的吧，cluster 这边有时候 lint 要耗时 10+min 的，所以我这边都在改成了 20min

Copy link
Member

Choose a reason for hiding this comment

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

各组件是可以自己改，不过如果时间太长的话，就得想办法优化。我觉得低于 5 min 是可以接受的。

Copy link

Choose a reason for hiding this comment

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

Is timeout: 5m OK for the template? @supereagle @lsytj0413 @Meoop

bbbmj
bbbmj reviewed Oct 25, 2019
View reviewed changes
Makefile

$(GOLANGCI_LINT):
curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(BIN_DIR) v1.16.0
curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(BIN_DIR) v1.20.1
Copy link
Member

Choose a reason for hiding this comment

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

是不是把 version 这玩意整成一个变量，放到前面去

Copy link
Member

Choose a reason for hiding this comment

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

只有一个地方用到，设不设变量无所谓

Copy link
Member

Choose a reason for hiding this comment

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

关键是保不齐下次又要改新版本。。。用变量传递的话，可以做到流水线直接传递。。就不需要改了

Copy link
Contributor

Choose a reason for hiding this comment

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

如果已经存在 golangci-lint 这个文件，更新版本号，$(GOLANGCI_LINT): 不会再执行的。想要用流水线控制下载的版本，那需要 golangci-lint 文件不存在，每次都拉。

Copy link
Member

Choose a reason for hiding this comment

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

流水线更新 golang:1.12.9-stretch 镜像 就可以了。

为了保证一致性，Makefile 还是得更新

Copy link

@hanxueluo hanxueluo Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

我个人建议，不要从外部以变量的方式传递到Makefile里。 编译整套系统（包括里面的工具）最好是紧跟代码（分支）走。（虽然当前我们受限于流水线系统也没真正做到，但自己能控制的尽量自己控制住）
否则，外部lint工具做了更新扫出新的一顿错误，那历史分支就挂了。

Copy link

Choose a reason for hiding this comment

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

恩，对了，还有 golang 版本谁在关注呢？k8s 有更新 golang 版本 kubernetes/kubernetes#84064

不用跟得这么紧吧。。呼唤钊总 @iawia002

这个节奏还是你 @supereagle 定一下吧，proposal 里面写的是 follow 我们平台 k8 使用的 go 版本，但是我们平台的 k8 从来不升级小版本的。

  • 如果 follow 具体的 k8 小版本，比如现在的 1.14.2，那 go 基本是一个大的 cps 版本升级一次
  • 如果 follow 大版本，比如现在的 1.14.x，go 版本一个 cps 版本中估计要改好几次，因为 k8 基本在追最新的 go 版本（1.14.x --> 1.12.x），这样相当于我们也要一直追一个最新的 go 版本

Copy link
Member

Choose a reason for hiding this comment

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

我个人建议，不要从外部以变量的方式传递到Makefile里。

同意

这个节奏还是你 @supereagle 定一下吧，proposal 里面写的是 follow 我们平台 k8 使用的 go 版本，但是我们平台的 k8 从来不升级小版本的。

那就 follow k8s 的小版本吧，我们没有必要跟的这么紧，成本太高了

Copy link
Contributor

@Meoop Meoop Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

很多更新的主要原因都是有涉及到安全的 bug 导致的，例如 1.12.9 ->1.12.10 修复的一个比较严重的 CVE-2019-16276（net/http 的 bug，在一些配置情况下可以绕过 k8s 身份认证）。

Copy link
Member

@supereagle supereagle Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ijumps 对于上面提到的安全问题，我们在 2.10 中打算升级 k8s 小版本吗？

@hezhizhen
Copy link

1.20.1 确定了吗？

@zhujian7
Copy link
Contributor Author

zhujian7 commented Nov 6, 2019

1.20.1 确定了吗？

现在提议是 1.20.1，如果没有人提 1.20.1 有啥问题，那就是 1.20.1 了。

@hezhizhen
Copy link

@Meoop 说 deadline 已经废弃了，要用 timeout 替换。这个也要修改吧

@supereagle
Copy link
Member

@Meoop 说 deadline 已经废弃了，要用 timeout 替换。这个也要修改吧

先不做修改，避免引入同步的工作量。等下次版本再次升级的时候，再修改，可能 lint config 也会进一步优化。

@supereagle
Copy link
Member

/lgtm

@caicloud-bot caicloud-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2019
@hezhizhen
Copy link

@Meoop 说 deadline 已经废弃了，要用 timeout 替换。这个也要修改吧

先不做修改，避免引入同步的工作量。等下次版本再次升级的时候，再修改，可能 lint config 也会进一步优化。

1.16 -> 1.20.1 不用同步吗？而且只是把 deadline 替换为 timeout，并不修改其他任何内容

@supereagle
Copy link
Member

1.16 -> 1.20.1 不用同步吗？而且只是把 deadline 替换为 timeout，并不修改其他任何内容

1.16 -> 1.20.1 需要同步，但是同步的内容仅是 makefile。

deadline 目前还能继续使用，没有替换的必要。下次升级版本或者优化 lint config 一起更新掉。

@hezhizhen
Copy link

1.16 -> 1.20.1 不用同步吗？而且只是把 deadline 替换为 timeout，并不修改其他任何内容

1.16 -> 1.20.1 需要同步，但是同步的内容仅是 makefile。

deadline 目前还能继续使用，没有替换的必要。下次升级版本或者优化 lint config 一起更新掉。

clear

@bbbmj
Copy link
Member

bbbmj commented Nov 8, 2019

/approve

@caicloud-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbbmj, supereagle

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

@caicloud-bot caicloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2019
@caicloud-bot caicloud-bot merged commit d40631f into caicloud:master Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbbmj bbbmj bbbmj left review comments

@iawia002 iawia002 iawia002 left review comments

@hezhizhen hezhizhen hezhizhen left review comments

@lsytj0413 lsytj0413 lsytj0413 left review comments

@Meoop Meoop Meoop left review comments

@supereagle supereagle supereagle left review comments

@hanxueluo hanxueluo hanxueluo left review comments

Assignees

@supereagle supereagle

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@zhujian7 @hezhizhen @supereagle @bbbmj @caicloud-bot @hanxueluo @Meoop @lsytj0413 @iawia002