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

Feature request: run linter a few times #449

Open
kolyshkin opened this issue Apr 12, 2022 · 5 comments
Open

Feature request: run linter a few times #449

kolyshkin opened this issue Apr 12, 2022 · 5 comments

Comments

@kolyshkin
Copy link

In my repository (https://github.com/opencontainers/runc) I use two distinct linter configs:

  • one where all default linters, and some non-default ones, are enabled
  • one where only a couple of extra linters are enabled

The first config is used everywhere (on main and release branches, as well as for PRs)

The second config is

  • aimed at new code only (and so it has only-new-issues: true)
  • aimed at PRs only (and so it has if: github.event_name == 'pull_request')

The idea behind this setup is simple: since we have a pretty big codebase, we can't possibly modify it to satisfy some more stricter and/or opinionated linters (such as godot, revive, gocritic, or dupl), nor it makes sense to do so -- but we can impose stricter standards for the newly added code.

Now, the GHA CI implementation (with the .golangci-extra.yml file with extra linters) looks like this:

jobs:

  lint:
    runs-on: ubuntu-20.04
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-go@v3
        with:
          go-version: "${{ env.GO_VERSION }}"
      - name: install deps
        run: |
          sudo apt -q update
          sudo apt -q install libseccomp-dev
      - uses: golangci/golangci-lint-action@v3
        with:
          version: "${{ env.LINT_VERSION }}"
      # Extra linters, only checking new code from pull requests.
      - uses: golangci/golangci-lint-action@v3
        if: github.event_name == 'pull_request'
        with:
          only-new-issues: true
          args: --config .golangci-extra.yml
          version: "${{ env.LINT_VERSION }}"

All this works, except that since I use golangci/golangci-lint-action twice, it actually downloads and installs golangci-lint twice (for example, see the log at https://github.com/opencontainers/runc/runs/5982042602?check_suite_focus=true.

** Proposal ** modify the action to detect that it is being used for the second time, and skip the second, redundant golangci-lint setup.

@SVilgelm
Copy link
Member

Actually you don't need to use the action on second run. You can call golangci-lint directly:

jobs:

  lint:
    runs-on: ubuntu-20.04
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-go@v3
        with:
          go-version: "${{ env.GO_VERSION }}"
      - name: install deps
        run: |
          sudo apt -q update
          sudo apt -q install libseccomp-dev
      - uses: golangci/golangci-lint-action@v3
        with:
          version: "${{ env.LINT_VERSION }}"
      # Extra linters, only checking new code from pull requests.
      - run: golangci-lint --config .golangci-extra.yml --new-from-rev=origin/${{ github.base_ref }} --out-format=github-actions

@kolyshkin
Copy link
Author

Actually you don't need to use the action on second run. You can call golangci-lint directly.

Awesome, thanks, let me check this

@kolyshkin
Copy link
Author

Actually you don't need to use the action on second run. You can call golangci-lint directly.

Awesome, thanks, let me check this

OTOH it would still be nice to have something like extra-run: which would do the same without the need to specify extra options manually.

@kolyshkin
Copy link
Author

For some reason it's not working (quoting https://github.com/opencontainers/runc/runs/6017736006?check_suite_focus=true):

Run golangci-lint run --config .golangci-extra.yml --new-from-rev=origin/main --out-format=github-actions
level=warning msg="[runner] Can't process result by diff processor: can't prepare diff by revgrep: could not read git repo: error executing git diff \"origin/main\" \"\": exit status 128"

I also see that the implementation of only-new-issues does some more than setting --new-from-rev (see

async function fetchPatch(): Promise<string> {
const onlyNewIssues = core.getInput(`only-new-issues`, { required: true }).trim()
if (onlyNewIssues !== `false` && onlyNewIssues !== `true`) {
throw new Error(`invalid value of "only-new-issues": "${onlyNewIssues}", expected "true" or "false"`)
}
if (onlyNewIssues === `false`) {
return ``
}
const ctx = github.context
if (ctx.eventName !== `pull_request`) {
core.info(`Not fetching patch for showing only new issues because it's not a pull request context: event name is ${ctx.eventName}`)
return ``
}
const pull = ctx.payload.pull_request
if (!pull) {
core.warning(`No pull request in context`)
return ``
}
const octokit = github.getOctokit(core.getInput(`github-token`, { required: true }))
let patch: string
try {
const patchResp = await octokit.rest.pulls.get({
owner: ctx.repo.owner,
repo: ctx.repo.repo,
[`pull_number`]: pull.number,
mediaType: {
format: `diff`,
},
})
if (patchResp.status !== 200) {
core.warning(`failed to fetch pull request patch: response status is ${patchResp.status}`)
return `` // don't fail the action, but analyze without patch
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
patch = patchResp.data as any
} catch (err) {
console.warn(`failed to fetch pull request patch:`, err)
return `` // don't fail the action, but analyze without patch
}
try {
const tempDir = await createTempDir()
const patchPath = path.join(tempDir, "pull.patch")
core.info(`Writing patch to ${patchPath}`)
await writeFile(patchPath, patch)
return patchPath
} catch (err) {
console.warn(`failed to save pull request patch:`, err)
return `` // don't fail the action, but analyze without patch
}
}
) -- apparently it fetches the PR diff.

I guess I can use --new-from-rev=origin/main if I check out the repo in full (i.e. add fetch-depth: 0 parameter to actions/checkout, but that would be an overkill.

kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 14, 2022
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Author

kolyshkin commented Apr 14, 2022

OK, made it working, see opencontainers/runc#3457

It should, you need to have something like

jobs:

  lint:
    runs-on: ubuntu-20.04
    steps:
      - uses: actions/checkout@v3
        with:
          fetch-depth: 2 # So we can do git diff HEAD~1
      - uses: actions/setup-go@v3
        with:
          go-version: "${{ env.GO_VERSION }}"
      - name: install deps
        run: |
          sudo apt -q update
          sudo apt -q install libseccomp-dev
      - uses: golangci/golangci-lint-action@v3
        with:
          version: v1.45
      # Extra linters, only checking new code from a pull request.
      - run: golangci-lint run --config .golangci-extra.yml --new-from-rev=HEAD~1 --out-format=github-actions

This relies on the fact that the last commit being fetched is the merge commit (somehow created by github) which shows the full diff for the PR.

kolyshkin added a commit to kolyshkin/runc that referenced this issue Mar 28, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 3, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 3, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 4, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 5, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 5, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 5, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this issue Apr 5, 2023
There is no need to parallelize lint and lint-extra jobs,
and they only differ with the arguments to golangci-lint.
Given that the longest time spent in these jobs is installing
libseccomp-dev, and that the second linter run can probably
benefit a lot from caching, it makes sense to merge them.

Move lint-extra from a separate job to a step in lint job.

The implementation is motivated by [1] and relies on the fact
that the last commit being fetched is the merge commit. So,
we need to set fetch-depth to 2 to be able to see the diff of
the merge commit -- and this is what golangci-lint is using.

[1] golangci/golangci-lint-action#449 (comment)

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit fa83a17)
Signed-off-by: Kir Kolyshkin <[email protected]>
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

No branches or pull requests

2 participants