detekt / detekt

Static code analysis for Kotlin
https://detekt.dev
Apache License 2.0
6.09k stars 755 forks source link

Add the ability to specify a detekt config from a remote artifact #5637

Open eygraber opened 1 year ago

eygraber commented 1 year ago

Expected Behavior

I'd like to be able to specify a detekt config from a remote artifact (e.g. maven).

Current Behavior

A detekt config has to be specified from a local file. It is possible to use a remote artifact in some scenarios, but it neither simple nor straightforward.

Context

I manage 30+ projects that use detekt. Whenever there's a detekt update (new rules, updated config, config deprecations, etc...) I have to update the config in all of those projects. It's a very manual, labor intensive task.

If I was able to have a single detekt config file that I publish to a maven repo, it would greatly simplify the process. There would be a one time cost of specifying the maven coordinates for the artifact, and that would be it. Leveraging Maven's versioning makes it even simpler, as I could update the config for a specific repo when I'm ready to. Tools like renovate and dependabot work very well with this since they'll create a PR automatically when I publish an update to the remote config.

If a project needed to change the config, it could simply provide a local config file to overlay the remote one.

eygraber commented 1 year ago

Took a quick look and I think this could be done by having a detektConfigs Gradle configuration the same way that there's a detektPlugins Gradle configuration.

So the user could specify something like this in their build file:

dependencies {
  detektConfigs("com.example:detekt-config:1.2.3")
}
BraisGabin commented 1 year ago

That feels like a cool api to me.

We could even use something like this to distribute our default configuration. Right now we distribute it inside the core module and it's kind of odd. The core shouldn't know about the rules.

eygraber commented 1 year ago

I'll try to put a PR together sometime this week (if anyone wants to beat me to it, feel free :smile: ).

cortinico commented 1 year ago

This sounds like a great addition. A couple of things to handle:

  1. What happens if more than one detektConfigs is specified? If you use the file config they're merged bottom-up. Configurations don't have the concept of ordering instead.
  2. What happens if you specify both a detektConfigs and a file config?
mpetuska commented 1 year ago
  1. Error. Alternatively it might be better to instead provide files like dsl for configs that detekt would add to some internal-only configuration. That would allow for ordering.
  2. I'd expect file to override remote configs
eygraber commented 1 year ago
  1. Looks like they'll be kept in the order declared in the dependencies block although possibly not if you dependencies.add("detektConfigs", ...)
  2. Same as @mpetuska
3flex commented 1 year ago

The Gradle APIs support this pretty easily:

val detektConfig by configurations.creating

dependencies {
    detektConfig("com.example:detekt-config:1.2.3")
}

detekt {
    config.from(resources.text.fromArchiveEntry(detektConfig, "path/to/detekt.yml"))
}

I'd suggest this route instead of building something custom in the plugin.

TextResourceFactory also supports pulling from remote HTTP(S) endpoints, which saves the publishing step, but wouldn't be supported with dependabot/renovate etc.:

detekt {
    config.from(resources.text.fromUri("https://detekt.dev/default-config.yml"))
}
eygraber commented 1 year ago

I was planning on the former. Should I also build support for the latter?

mpetuska commented 1 year ago

I think the latter would be awesome to have too. Although I'd expect some sugar DSL in detekt block to hide gradle's internals for resources call. I'd even add that to existing files api since it's basically just a remote file afterall.

cortinico commented 1 year ago

TextResourceFactory also supports pulling from remote HTTP(S) endpoints, which saves the publishing step, but wouldn't be supported with dependabot/renovate etc.:

+1 to this. I recall that Gradle had some support for this. I've tried to Google this but this was quite of a challenge to find. Also what are the implications of using fromUri on Gradle caching mechanisms?

3flex commented 1 year ago

I was planning on the former. Should I also build support for the latter?

Apologies if I wasn't clear - since this is trivial to do with Gradle using the code I posted, I don't think we should build anything into the detekt Gradle plugin.

gradle's internals for resources call

These aren't internal APIs. They're part of the public Gradle API.

what are the implications of using fromUri on Gradle caching mechanisms?

Not sure... Creating a configuration and using fromArchiveEntry would be most reliable from a caching perspective. fromUri doesn't mention caching behaviour in its documentation.

eygraber commented 1 year ago

OK I didn't get that you were saying not to support it in the plugin.

I disagree with that though. I think it's a very useful feature, and asking users to set up a configuration and then consume it is a high barrier to entry (even if it is simple, most users have never done that before).

I'll defer to you though, and if you don't want it in the plugin then I guess the next best thing would be documenting that somewhere?

mpetuska commented 1 year ago

+1 to the above. I think providing concise dsl that utilizes mentioned gradle features brings substantial value for low cost.

3flex commented 1 year ago

asking users to set up a configuration... even if it is simple, most users have never done that before

While this is true, this is only needed by those who are also creating their own published artifact that contains a custom detekt configuration file. This is also something most users haven't done before.

substantial value for low cost

Remember that if this is added to the detekt Gradle plugin, we need:

I don't see the value when Gradle already has the ability for someone to set this up exactly how they like, with the ordering they want, using the existing DSL, in just a couple of lines.

eygraber commented 1 year ago

This is also something most users haven't done before.

I think if this is supported by the plugin it would be more likely for that to happen. Same story with detektPlugins. It's there and easy so it's used. There could even be a marketplace for configs like there is for plugins.

Some way to deal with multiple artifacts provided in the same configuration (error, or some custom ordering logic)

From the link I posted above it seems that they'll be in the order used in the dependencies block (uses an ordered set; can check with the gradle team if that's spec or an implementation detail).

Logic to prefer "local" files over the remote files

Should just be the order that from is called (which I'll take care of in my pr)

Test

I'll write the tests

To maintain the feature over time + Document

I hear that, but like you said, it's not that complex and will be good for users. If this was a crazy complex feature that was only tangentially related to detekt I'd agree with you.

In any case, let me know if you change your mind and I'll put something together in my free time. Maybe I'll do it anyways so there's something to look at when making the decision to merge or not.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

TWiStErRob commented 1 year ago

Could we settle this with a short note/section in the https://detekt.dev/docs/next/gettingstarted/gradle page listing the official Gradle API usage @3flex mentioned?

cortinico commented 1 year ago

Could we settle this with a short note/section in the detekt.dev/docs/next/gettingstarted/gradle page listing the official Gradle API usage @3flex mentioned?

Yup this could be added 👍 perhaps even as a 'advanced use cases' page to don't bloat the Gradle page already?

detekt-ci commented 11 months ago

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

mpetuska commented 11 months ago

.

cortinico commented 10 months ago

Just a heads up that this can be fixed just by updating the detekt documentation, if someone wants to help us with it