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

ci: add golangci-lint, yamlllint and codespell #79

Merged
merged 1 commit into from
Jun 15, 2022
Merged

ci: add golangci-lint, yamlllint and codespell #79

merged 1 commit into from
Jun 15, 2022

Conversation

lianghao208
Copy link
Contributor

@lianghao208 lianghao208 commented May 28, 2022 •
edited
Loading

Signed-off-by: lianghao208 roylizard3@gmail.com
issue: #73

@lianghao208 lianghao208 force-pushed the golint branch 2 times, most recently from fe354ec to dae2e0e Compare May 28, 2022 03:18
youngnick
youngnick previously approved these changes May 31, 2022
View reviewed changes
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

This is looking great, nice work @lianghao208!

.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
hack/golangci-lint Outdated Show resolved Hide resolved
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
hack/codespell.sh Outdated Show resolved Hide resolved
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
hack/golangci-lint Outdated Show resolved Hide resolved
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
hack/golangci-lint Outdated Show resolved Hide resolved
@lianghao208 lianghao208 mentioned this pull request Jun 1, 2022
Closed
@danehans
Copy link
Contributor

danehans commented Jun 2, 2022

@lianghao208 thanks again for the PR. When you have a moment, can you update the PR base on the feedback?

@lianghao208
Copy link
Contributor Author

@lianghao208 thanks again for the PR. When you have a moment, can you update the PR base on the feedback?

@danehans I've already update the PR 2 days ago, and there seems to be some details haven't been confirmed yet.

ping @arkodg

@arkodg
Copy link
Contributor

arkodg commented Jun 3, 2022

@lianghao208 raised #90 to make a call on how we install tooling
for the project, the decision made in the issue should help drive the implementation of this PR

@lianghao208 lianghao208 force-pushed the golint branch 3 times, most recently from e2dbe1d to e1684cd Compare June 3, 2022 08:17
@lianghao208
Copy link
Contributor Author

In #90 (comment), we reached a consensus. Let's keep the hack approach and make this PR merged.
@arkodg @youngnick

arkodg
arkodg reviewed Jun 8, 2022
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
arkodg
arkodg reviewed Jun 8, 2022
View reviewed changes
hack/yamllint.sh Outdated Show resolved Hide resolved
arkodg
arkodg reviewed Jun 8, 2022
View reviewed changes
hack/codespell.sh Outdated Show resolved Hide resolved
@danehans
Copy link
Contributor

danehans commented Jun 8, 2022

xref #63

@arkodg arkodg mentioned this pull request Jun 8, 2022
Closed
@lianghao208
Copy link
Contributor Author

Since #102 is still in progress, can we separate this into two PRs?
This PR is for git workflow only, once #102 has been merged, I will work on another for the dev side(Makefile).
@arkodg @danehans

@arkodg
Copy link
Contributor

arkodg commented Jun 10, 2022

@lianghao208 #102 is now in, which should unblock this PR
recommend moving any tools files such as .codespell into tools/<toolname>/ , TIA

@lianghao208 lianghao208 marked this pull request as draft June 11, 2022 08:45
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
tools/codespell/Dockerfile.builder Outdated
@@ -0,0 +1,8 @@
# python
FROM python:3.7-slim as python
Copy link
Contributor

Choose a reason for hiding this comment

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

you will need to install codespell and yamllint in tools/Dockerfile.builder

Copy link
Contributor Author

@lianghao208 lianghao208 Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@arkodg
Sorry for my miscomprehension, I thought the linters need to run as docker-in-docker(cause the tools/Dockerfile.builder only provides golang base image and has docker installed)

If we install codespell and yamllint in tools/Dockerfile.builder, does it mean we need a ubuntu base image instead of golang? (python is required for yamllint and codespell)

FROM golang:1.18.2 as builder > FROM ubuntu:focal as builder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refer to Istio: https://github.com/istio/tools/blob/master/docker/build-tools/Dockerfile

I made some changes.

arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
tools/yamllint/yamllint.sh Outdated Show resolved Hide resolved
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
DEVELOPER.md Show resolved Hide resolved
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
.yamllint Outdated Show resolved Hide resolved
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
.codespell.ignorewords Outdated Show resolved Hide resolved
@lianghao208 lianghao208 force-pushed the golint branch 4 times, most recently from da6e849 to 4d072de Compare June 14, 2022 12:20
@lianghao208
Copy link
Contributor Author

@arkodg
Already updated, PTAL, thanks.

ps: I think each linter runs on separate containers saves much more time than running on single tools/Dockerfile.builder to set up environment step by step. But since istio and envoy are using this method, I guess this is a widely accepted approach.

@danehans
Copy link
Contributor

@lizan thoughts regarding #79 (comment)?

arkodg
arkodg reviewed Jun 14, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
arkodg
arkodg reviewed Jun 14, 2022
View reviewed changes
tools/Dockerfile.builder Outdated Show resolved Hide resolved
@lianghao208 lianghao208 force-pushed the golint branch 3 times, most recently from 74c2000 to 5fa4c7c Compare June 14, 2022 17:20
@lianghao208
ci: add golangci-lint, yamlllint and codespell
dcfd476 
Signed-off-by: lianghao208 <roylizard3@gmail.com>
@lianghao208
Copy link
Contributor Author

lianghao208 commented Jun 15, 2022 •
edited
Loading

/need-approval-to-run-workflows
@arkodg @danehans

@codecov-commenter
Copy link

Codecov Report

Merging #79 (dcfd476) into main (08b8eac) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main       #79   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            5         5           
=========================================
  Hits             5         5

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 08b8eac...dcfd476. Read the comment docs.

arkodg
arkodg approved these changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working through the review comments !

Makefile Show resolved Hide resolved
@arkodg arkodg requested review from skriss and youngnick June 15, 2022 15:53
@danehans danehans merged commit b2d95ac into envoyproxy:main Jun 15, 2022
@danehans danehans mentioned this pull request Jun 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danehans danehans danehans left review comments

@arkodg arkodg arkodg approved these changes

@skriss skriss Awaiting requested review from skriss

@youngnick youngnick Awaiting requested review from youngnick

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lianghao208 @danehans @arkodg @codecov-commenter @youngnick