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

feat: sharable config presets #2241

Closed
wants to merge 1 commit into from
Closed

feat: sharable config presets #2241

wants to merge 1 commit into from

Conversation

thepwagner
Copy link

Add an implementation of "shareable presets", as discussed by #1141 .

This is configured by a new root element, like:

presets:
- ./.golangci.myorg.yml  # an additional configuration file on disk, maybe maintained via curl
- github.com/thepwagner/golangci-lint-yaml # a go module, which must contain a configuration file

That feature set is aiming to align with the opinions in #2137 - minimize additional remote access by delegating to the go module system. Presets can be pinned in go.sum, fetched with go mod download, vendored, change-managed by Dependabot, etc. This mirrors alternatives like eslint-config-airbnb. Authentication is delegated to go modules: I've tested using ssh and a private repo.

Presets are not loaded recursively, to limit overhead and complexity.

Preset configurations are merged rather ungracefully: viper does the heavy lifting. The exception is the .linters.enabled and .linters.disabled lists, which are hand-merged to avoid the most expected repetition. This merge operation is the biggest area for improvements.

The included test references a public repo that worksonmymachine(TM), this is just for a demo and should be replaced with something embedded or controlled by this org.
thepwagner/archivist#130 is a standalone sample of the changes required to a project to adopt a shared preset. The ugly bit is treating the lint configuration as a Go dependency.

@boring-cyborg
Copy link

boring-cyborg bot commented Sep 18, 2021

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Sep 18, 2021

CLA assistant check
All committers have signed the CLA.

@ldez ldez self-requested a review September 18, 2021 14:35
@ldez ldez added the blocked Need's direct action from maintainer label Sep 18, 2021
@ldez ldez marked this pull request as draft September 18, 2021 14:45
@@ -0,0 +1,3 @@
# TODO: host a preset in the golangci org?
presets:
- github.com/thepwagner/golangci-lint-yaml
Copy link

Choose a reason for hiding this comment

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

Could it be located in golangci-lint/test/testdata ?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, but I'm not sold on that path: the goal of this test is to exercise loading the configuration from a go module.

Using a replace directive, we could load from a go module that happens to be housed at ./test/testdata/...:

require github.com/golangci/golangci-lint-yaml v0.0.1
replace github.com/golangci/golangci-lint-yaml => ./test/testdata/some-new-dir

I figured since #1141 mentions "presets like golangci:recommended", using such a preset here would be the best solution. To shoe-horn that idea into this proposal, a module like github.com/golangci/golangci-recommended would be the equivalent of golangci:recommended preset.


Any repo with a .golangci.yml should work - using an existing dependency like github.com/ldez/gomoddirectives would have been less egregious to the dependency graph, but I thought the pattern of a module containing only the configuration was more interesting to include in the proposal.

Copy link

Choose a reason for hiding this comment

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

I see.

In my case I would like have any github or https path (not exactly the go module or some convention of names).

presets:
  - local/path/config.yml
  - https://raw.githubusercontent.com/golangci/golangci-lint/master/.pre-commit-hooks.yaml
  - github:golangci/golangci-lint/master/subfolder/docs/common.yaml

Copy link
Author

Choose a reason for hiding this comment

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

any https path

Per #2137 (comment) , I don't think https:// paths would be a welcome addition. If you want an HTTPS resource, they can be curl-ed first and used as a file preset.

any github path

That might be useful, e.g. to have multiple configurations in a single repository.

Would it be acceptable for that path to also be a valid go module? Again, I wanted to avoid adding outgoing network calls, authentication etc. Only using files provided by go mod is an attempt to walk that line.

Copy link

Choose a reason for hiding this comment

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

It make sense. Thank you for the hint.

In this case then I would propose first support only local path and think about go mod solution in separate PR.

presets := make([]*viper.Viper, 0, len(r.cfg.Presets))
for _, preset := range r.cfg.Presets {
presetV := viper.New()
if strings.HasPrefix(preset, ".") {
Copy link

Choose a reason for hiding this comment

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

It means the local path always should be started with ./ ?

Copy link
Author

Choose a reason for hiding this comment

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

Or ../. The pattern for distinguishing modules from local paths is inspired by the right hand side of replace directives in go.mod.

If the path on the right side of the arrow is an absolute or relative path (beginning with ./ or ../), it is interpreted as the local file path to the replacement module root directory, which must contain a go.mod file.
[...]
If the path on the right side is not a local path, it must be a valid module path.

/ might also be a valid prefix to trigger file loading that I missed did not think would be used.

@thepwagner
Copy link
Author

@ldez do you have any feedback on this?

@thepwagner thepwagner closed this Oct 22, 2021
@Nivl
Copy link
Contributor

Nivl commented Jun 26, 2023

I know this is old but I do like this idea. This solves a bunch of issues for organizations with hundreds of repos. The breaking changes in 1.53 (like #3906) would have been much more manageable with something like this in place.

Not a huge fan of having to hardcode the packages in a lint.go file though but it's not terrible either. At the very least a V1 that supports merging local files would go really far. Users can use curl, submodules, etc. to bring in anything else in the short term.

@ldez I see that you've set yourself as reviewer. Do you have any opinions about it? I don't mind opening a PR to work/help on that depending on what @thepwagner wants to do. I just want to make sure it'll go somewhere before I (or others) start working on anything.

@ldez
Copy link
Member

ldez commented Jun 26, 2023

This implementation based on external files will create problems, for example with the invalidation of the cache, another is related to security with URL.

It's not the first PR on this topic and the problems are always the same.

So before working on something, I recommend making proposals inside #1141.

@ldez ldez added the area: config Related to .golangci.yml and/or cli options label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: config Related to .golangci.yml and/or cli options blocked Need's direct action from maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants