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

fix: add golangci-lint setup action and fix lint issue #592

Merged
merged 1 commit into from
May 28, 2024

Conversation

ndajr
Copy link
Contributor

@ndajr ndajr commented May 28, 2024

The make check linting step failed: https://github.com/cosmtrek/air/actions/runs/9240465156/job/25420774320. The build step running in each PR is broken with golangci-lint: command not found issue: https://github.com/cosmtrek/air/actions/runs/9149739894/job/25364966455#step:4:13

Copy link

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 68.89%. Comparing base (f673320) to head (90649c1).

Files Patch % Lines
runner/proxy.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #592      +/-   ##
==========================================
+ Coverage   68.49%   68.89%   +0.39%     
==========================================
  Files          11       11              
  Lines        1073     1061      -12     
==========================================
- Hits          735      731       -4     
+ Misses        247      243       -4     
+ Partials       91       87       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xiantang xiantang merged commit e3b6eaf into air-verse:master May 28, 2024
8 of 9 checks passed
@ndajr ndajr deleted the fix-pipeline branch May 28, 2024 09:58
- name: Install golangci-lint
uses: golangci/golangci-lint-action@v6
with:
version: v1.59
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip: You could have used "latest" so you don't have to maintain it

Copy link

Choose a reason for hiding this comment

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

fyi, Same reply from the other comment, but i'll throw in tools like renovate exist so you can automate updating things, while keeping consistent build experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure dependabot cannot touch this argument of an action. It can bump the action version. But not its argument.

But maybe renovate can do it, so it would be fine

@@ -20,11 +20,14 @@ jobs:
uses: actions/setup-go@v4
with:
go-version: ^1.22
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to your change but related to my other comment

#592 (comment)

Here you can use "stable" it will always use the latest version and you don't have to maintain it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could but it has downsides as well. It may be "easier" to maintain but may break all the users of the project, which has a bigger impact than just change the version in every few months. See https://vsupalov.com/docker-latest-tag/

Copy link
Contributor

Choose a reason for hiding this comment

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

We could but it has downsides as well. It may be "easier" to maintain but may break all the users of the project, which has a bigger impact than just change the version in every few months. See https://vsupalov.com/docker-latest-tag/

Very good read, I get it.

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