Skip to content

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

Merged
danehans merged 1 commit intoenvoyproxy:mainfrom
lianghao208:golint
Jun 15, 2022
Merged

ci: add golangci-lint, yamlllint and codespell#79
danehans merged 1 commit intoenvoyproxy:mainfrom
lianghao208:golint

Conversation

@lianghao208
Copy link
Copy Markdown
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
Copy Markdown
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!

arkodg
arkodg reviewed May 31, 2022
View reviewed changes
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
arkodg
arkodg reviewed May 31, 2022
View reviewed changes
@lianghao208 lianghao208 mentioned this pull request Jun 1, 2022
Closed
@danehans
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

lianghao208 commented Jun 3, 2022

@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
Copy Markdown
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
Copy Markdown
Contributor Author

lianghao208 commented Jun 7, 2022

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
arkodg
arkodg reviewed Jun 8, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 8, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 8, 2022
View reviewed changes
@danehans
Copy link
Copy Markdown
Contributor

danehans commented Jun 8, 2022

xref #63

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

lianghao208 commented Jun 9, 2022

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
arkodg reviewed Jun 10, 2022
View reviewed changes
@arkodg
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor

@arkodg arkodg Jun 13, 2022

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
Copy Markdown
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
Copy Markdown
Contributor Author

@lianghao208 lianghao208 Jun 14, 2022

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
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 13, 2022
View reviewed changes
@lianghao208 lianghao208 force-pushed the golint branch 4 times, most recently from da6e849 to 4d072de Compare June 14, 2022 12:20
@lianghao208
Copy link
Copy Markdown
Contributor Author

lianghao208 commented Jun 14, 2022

@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
Copy Markdown
Contributor

danehans commented Jun 14, 2022

@lizan thoughts regarding #79 (comment)?

arkodg
arkodg reviewed Jun 14, 2022
View reviewed changes
arkodg
arkodg reviewed Jun 14, 2022
View reviewed changes
@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
Copy Markdown
Contributor Author

lianghao208 commented Jun 15, 2022 •
edited
Loading

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

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 15, 2022

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
Copy Markdown
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 !

@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

@arkodg arkodg arkodg approved these changes

@skriss skriss Awaiting requested review from skriss

@youngnick youngnick Awaiting requested review from youngnick

+1 more reviewer

@danehans danehans danehans left review comments

Reviewers whose approvals may not affect merge requirements

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