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

Add support for new-from-rev #820

Closed
2 tasks done
flimzy opened this issue Aug 10, 2023 · 3 comments
Closed
2 tasks done

Add support for new-from-rev #820

flimzy opened this issue Aug 10, 2023 · 3 comments
Labels
question Further information is requested

Comments

@flimzy
Copy link

flimzy commented Aug 10, 2023

Welcome

  • Yes, I understand that the GitHub action repository is not the repository of golangci-lint itself.
  • Yes, I've searched similar issues on GitHub and didn't find any.

Your feature request related to a problem? Please describe.

The GitHub action supports new-issues-only, but this is not a direct replacement for new-from-rev, and has caused a few problems on some projects I'm working on. Therefore, I'd like to request adding explicit support for new-from-rev in the GitHub Action as well. I am aware of issue #30, but as detailed below, new-issues-only does not offer quite the same behavior as new-from-rev.

Details

When the local golangci-lint is configured with the new-from-rev: <commit sha>, and PRs leapfrog each other, it is possible to have a PR introduce a lint failure that is not caught by new-issues-only, but is caught by new-from-rev, causing the local linter to fail, while GHA reports success.

This can happen when one PR that adds a Deprecated: notice, makes a function/type/const unused, or makes a function variable unused, or other scenarios as well, without touching the line that would trigger the lint warning, while another PR, later merged, does touch the line.

Example:

PR 1 removes the only reference to ctx

 func Foo(c context.Context) (string, error) {
-    str, err := DoThing(c)
-    if err != nil {
-        return "", err
-    }
-    return str, nil
+    return "default", nil
 }

Meanwhile, PR 2 touches the line that "should" trigger the linter in PR 1, but that lint warning was suppressed by the new-from-rev config option, because that line has not been changed since the linter was implemented.

-func Foo(c context.Context) (string, error) {
-    str, err := DoThing(c)
+func Foo(ctx context.Context) (string, error) {
+    str, err := DoThing(ctx)
     if err != nil {
         return "", err
     }
     return str, nil
 }

Both of these PRs pass the new-issues-only configuration option, but once they're both merged, the new-from-rev configuration option results in a lint failure.

Describe the solution you'd like.

Add a new-from-rev configuration option to the GitHub Action, to offer exact feature parity with the configuration file and CLI usage.

Describe alternatives you've considered.

The three alternatives I'm aware of:

  • Forego use of the GitHub Action, and call golangci-lint directly, the same way it's run locally. This has obvious drawbacks.
  • Force that all PRs are rebased before merge, rather than allowing the use of the default button in the GitHub interface to merge main into the feature branch when it's out of sync. This increases the workload on the team, and probably requires extra scripting to enforce/streamline.
  • Live with the problem as it is, and clean up the fallout when it's detected. This is the current situation we're in, and it leads to multiple devs fixing the newly uncovered lint, often in different ways, and creating merge conflits. This is what we want to avoid.

Additional context.

No response

@ozancaglayan
Copy link

ozancaglayan commented Oct 30, 2023

Just as a side note, I'm able to use this feature by passing it to args

args: --new-from-rev <rev>

@flimzy
Copy link
Author

flimzy commented Dec 1, 2023

That seems to have no effect for my repository.

@ldez
Copy link
Member

ldez commented Apr 29, 2024

The solution is to use, as suggested by @ozancaglayan args:

      - name: golangci-lint
        uses: golangci/golangci-lint-action@v5
        with:
...
          args: --new-from-rev=<rev>
...

Note the = is important because the action parses the arguments on spaces.

@ldez ldez added the question Further information is requested label Apr 29, 2024
@ldez ldez closed this as completed Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants