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 path prefix if working directory specified #34

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

jwilner
Copy link
Contributor

@jwilner jwilner commented Jul 10, 2020

Following up on #31 -- adds --path-prefix if working-directory is specified.

Worth noting that this PR and this option basically have a dependency on golangci/golangci-lint#1226

@SVilgelm
Copy link
Member

you are so fast! :-)
I have just one concern - Will it work if the working-directory is an absolute path?

Is it possible to test GitHub Actions by specifying a commit id instead of version?

@jwilner
Copy link
Contributor Author

jwilner commented Jul 10, 2020

@SVilgelm, even without path-prefix, I think specifying an absolute path for working-directory would be a surprising and brittle choice, since users would be coupling their build to the layout of the github action FS (which they don't control), rather than just the layout of their repository (which they do).

That said, nothing to stop people from doing it!

I think the path-prefix logic behaves reasonably in that scenario because it still prefers user override.

relative path: ✅

working-directory: subdir
example output path: subdir/path/to/file.go

A relative path is the most likely usage pattern imo, and it works exactly like you'd want (and what GH wants).

absolute path, no explicit --path-prefix: 🚫

working-directory: /foo/bar/myrepo/subdir
output paths: /foo/bar/myrepo/subdir/path/to/file.go

I don't believe this will work with github-actions, because the error paths will no longer be relative to the repo. If, for an unknown reason, the user needs the absolute path, they can specify the path prefix explicitly, as shown below.

absolute path, explicit --path-prefix: ✅

working-directory: /foo/bar/myrepo/subdir
args: '--path-prefix=subdir'
output paths: subdir/path/to/file.go

Because the logic prefers any explicit CLI path-prefix arg, the user still has the option to override the default behavior.

@SVilgelm SVilgelm requested a review from a team July 10, 2020 14:26
@SVilgelm
Copy link
Member

@jwilner thank you for the explanation, I'm totally agree with you.

Copy link

@iwankgb iwankgb left a comment

Choose a reason for hiding this comment

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

I don't think we can do much more besides merging it.

@iwankgb iwankgb merged commit cf72500 into golangci:master Jul 10, 2020
@SVilgelm
Copy link
Member

@iwankgb it's great that you merged it, but It depends on the golangci/golangci-lint#1226 pull request, what is not merged yet and the action can fail with an error:

Can't get config for command line: can't parse args: unknown flag: --path-prefix

@sayboras
Copy link
Member

I just merged golangci/golangci-lint#1226, but I don't think there is a way to point to golangci-lint master branch, so we need to wait for release as well ?

@iwankgb
Copy link

iwankgb commented Jul 11, 2020

Sorry, I have not realised it and have not read description carefully enough. Can any of you file a reverting PR, please?

@SVilgelm
Copy link
Member

@jwilner no worries, that PR and a fix for that PR are already merged to master :)

@jwilner
Copy link
Contributor Author

jwilner commented Jul 11, 2020

@SVilgelm what's the procedure for cutting a new release of golangci-lint? Seems like it'd just be a patch version.

@SVilgelm
Copy link
Member

SVilgelm commented Jul 11, 2020

@jwilner I never did it, probably we need to ask @sayboras. But according to the documentation[1] we just need to create a new tag.
But let's release the golangci-lint tool first, then the action

[1] https://golangci-lint.run/contributing/workflow/#new-releases

@iwankgb
Copy link

iwankgb commented Jul 11, 2020

Beyond the tag some documentation must be generated

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.

None yet

4 participants