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

Detekt Rules Marketplace #5191

Merged
merged 6 commits into from Sep 20, 2022
Merged

Detekt Rules Marketplace #5191

merged 6 commits into from Sep 20, 2022

Conversation

cortinico
Copy link
Member

@cortinico cortinico commented Aug 6, 2022

This is an early draft of my idea of a 'Rules Marketplace' which is hosted on our website. Users will just have to edit the array on rulesmarketplace.js via the button I added on the header.

I've already added 5 rulesets which I found online.

I'm looking into collecting some feedbacks on which fields we believe might be more useful for users to share. I've added a list of Rules as they will be indexed by Algolia so they will end up in the search results.

I haven't implemented a search bar as I was looking for a feedback on this proposal before developing more features.

Here a preview link: https://detekt-3usyfqpdp-detekt.vercel.app/marketplace

Fixes #4878

@cortinico cortinico marked this pull request as draft August 6, 2022 18:18
@codecov
Copy link

codecov bot commented Aug 6, 2022

Codecov Report

Merging #5191 (c7b7d82) into main (84276f2) will not change coverage.
The diff coverage is n/a.

❗ Current head c7b7d82 differs from pull request most recent head 8f69120. Consider uploading reports for the commit 8f69120 to get more accurate results

@@     Coverage Diff      @@
##   main   #5191   +/-   ##
============================
============================

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@marschwar
Copy link
Contributor

This is very cool. This will incentivise creating new rules and will reduce the number of times we decline adding a new first party rule.

One more thought: Should this be a general detekt plugin market place that also allows users to share other extensions such as processors and reports? I do not know if users actually use those extensions.

@cortinico
Copy link
Member Author

One more thought: Should this be a general detekt plugin market place that also allows users to share other extensions such as processors and reports? I do not know if users actually use those extensions.

Yup we could add a 'tag' to distinguish projects that include just rules or processors/reports.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Looks good!

One thought, maybe we can also move the 3rd party rules from the readme to the website.

Custom rules and reports from 3rd parties:

@3flex
Copy link
Member

3flex commented Aug 7, 2022

This looks great!

I just wonder about pros/cons of listing the specific rule name and whether the rules use type resolution or not... it puts extra effort on third party rule maintainers, contributors or us to keep this up to date. Perhaps that should be optional (if it's not already)?

@github-actions
Copy link

github-actions bot commented Aug 7, 2022

Messages
📖

Thank you very much for making our website better ❤️!

Generated by 🚫 dangerJS against 8f69120

@cortinico
Copy link
Member Author

Perhaps that should be optional (if it's not already)?

I can make this optional. I'm more worried of things getting out of sync (like rule authors would have to keep on updating the doc with new rules). Maybe that's not a problem at the moment given the low traffic 🤔

@3flex
Copy link
Member

3flex commented Aug 7, 2022

rule authors would have to keep on updating the doc with new rules

Yeah definitely that too.

It's not the end of the world if it's not kept up to date though. And the nice thing is that the rule list gives a really good idea of what sort of things the rule set is going to check without having to click through to the project site. And as you mentioned they'll be included in the search index which is a really nice feature.

Separately, again probably pros outweigh cons and I know why it was done, but having the dynamic + version for the dependency isn't best practice. Maybe asking people to click through to the project readme is better?

If the dependency coordinates are left in it might also be good to list the actual repository that users will need in their list, e.g. say mavenCentral() or the repo URL if it's GitHub packages or JitPack instead of just the repo name.

@BraisGabin
Copy link
Member

We could use something similar to #5080. My idea:

  • The rule authors only creates a PR pointing to their repository.
  • We have a nightly that checkout all those repos and run ./gradlew generateDocumentation and we use that output to populate the web.

Pros:

  • It's easy for the rule author to keep all up to date.
  • We could use this for our own first-party plugins.
    Cons:
  • We don't have full control on what we display on our website.

Given the low traffic I think that the second is not a big deal and we can always remove the repo from the list.

@cortinico
Copy link
Member Author

  • We have a nightly that checkout all those repos and run ./gradlew genTerateDocumentation and we use that output to populate the web.

I'm a bit against this approach for a number of reason:

  • I would refrain from checking out arbitrary code and running it on our CI (where our secrets could be accessible). It exposes arbitrary code execution in a trusted environment (our repo).
  • It adds maintenance cost on our shoulder (the nightly job might fail for a number of reasons).
  • We don't have a standardized project setup. Some repos have a gradle build in the top folder. Others have it in a subfolder. Some have examples project, etc.

I would rather ask rule authors to add a detekt-metadata.yml file in their repo, and we fetch infos from there. Still, this file has to be kept up to date. Then sending a PR against the website is probably the same amount of work.

@BraisGabin
Copy link
Member

You are completely right. I didn't think about the security concerns.

But I still think that we shouldn't be the gate keepers for changes in the marketplace. I assume that for a rule author it would be easier to push something in its repo than forking ours, pushing, create a PR and wait for an approval. So I would vote for the Metadata file.

@3flex
Copy link
Member

3flex commented Aug 11, 2022

I still think that we shouldn't be the gate keepers for changes in the marketplace

I think we should be gatekeepers, since the content appears on the detekt site which we're ultimately responsible for. I think it's rare to have marketplaces that don't have moderation of some kind.

For the initial version we can start with manual updates and perhaps pulling and parsing metadata from 3rd party repos can be considered later.

@cortinico
Copy link
Member Author

One more thought: Should this be a general detekt plugin market place that also allows users to share other extensions such as processors and reports? I do not know if users actually use those extensions.

I've added the tags now. The 4 available tags are "ruleset", "processor", "reporter", "plugin". I've also added a couple of rules more.

I just wonder about pros/cons of listing the specific rule name and whether the rules use type resolution or not... it puts extra effort on third party rule maintainers, contributors or us to keep this up to date. Perhaps that should be optional (if it's not already)?

Made them optional 👍

Separately, again probably pros outweigh cons and I know why it was done, but having the dynamic + version for the dependency isn't best practice. Maybe asking people to click through to the project readme is better?
If the dependency coordinates are left in it might also be good to list the actual repository that users will need in their list, e.g. say mavenCentral() or the repo URL if it's GitHub packages or JitPack instead of just the repo name.

I removed the whole Maven Coordinates/Maven Repo thing. I wasn't happy with how it would render. Plus I think users that want to use a rule will ultimately end up in their readme where the proper version can be displayed. A lot of rule were hosted on Github Packages which made the setup more complicated. Let me know if you folks are fine with this approach 👍

This is good to merge on my end 👍

@cortinico cortinico marked this pull request as ready for review September 11, 2022 21:32
@cortinico cortinico added this to the 1.22.0 milestone Sep 11, 2022
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Sep 11, 2022
@cortinico cortinico changed the title Initial draft of Rules Marketplace Detekt Rules Marketplace Sep 11, 2022
Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

nit: I think that the url gives value here. You see the name of the repo and who is behind it. What do you think about something like this:

Captura de pantalla 2022-09-12 a las 23 10 37


I really really like this initiative :)

website/src/pages/marketplace/index.jsx Show resolved Hide resolved
@cortinico
Copy link
Member Author

nit: I think that the url gives value here. You see the name of the repo and who is behind it. What do you think about something like this:

Yeah that's true 👍 I've added it. You can find in the next preview

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

LGTM :)

@cortinico
Copy link
Member Author

I've added https://github.com/twitter/compose-rules/
Are we good merging this?

@BraisGabin
Copy link
Member

LGTM

@3flex 3flex merged commit 8fe53b4 into detekt:main Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give visibility to the third-party detekt plugins
5 participants