-
-
Notifications
You must be signed in to change notification settings - Fork 758
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
Remove dependency on mavenLocal() for functionalTests #6060
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6060 +/- ##
=========================================
Coverage 84.86% 84.86%
Complexity 3975 3975
=========================================
Files 564 564
Lines 13300 13300
Branches 2325 2325
=========================================
Hits 11287 11287
Misses 870 870
Partials 1143 1143 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad that this pain point gets removed! 👍
Completely agree with the goal of removing mavenLocal usage, however this seems to make it more difficult to iterate on changes to CLI and the plugin and run functional tests. When doing something like adding a new CLI flag and a complementary property in the Gradle plugin it seems like there's no way to run the functional tests with the updated CLI dependency? Related (unmerged) PRs: |
Co-authored-by: Matthew Haughton <3flex@users.noreply.github.com>
Yeah I agree, but I believe this is an improvement from the current status. If it helps, I can setup a separate CI job that bumps the version to |
That would help on CI but not local builds - unless that new task is attached to the Maybe add that and then outline how one would update CLI & Gradle plugin together and run functional tests using that new task. As long as it's not necessary to update test code to get those tests working together when needed. |
I don't think we test this flow today with local build, though. As of today, local builds fetch They will end up using
|
I recently found a Gradle feature that would fix that: https://docs.gradle.org/current/userguide/declaring_repositories.html#declaring_content_exclusively_found_in_one_repository Using this means only Maven Local would be searched for the dependency. If the detekt CLI version is declared as a dynamic version then this would avoid the need to do any version bump as well. The only requirement then is pushing the required dependencies to Maven Local which can be easily setup with task dependencies. Would this resolve your release issues as well? |
Yup we could use this, yet still we will be forcing to depend on |
(holding this till after 1.23.0 stable) |
@3flex what's the best course of action for this? I'd really love to get rid of this setup as it's making the release process extremely complicated |
This PR is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days. |
I vote to merge. If in the future we find better options we can revert this one and implement the better option but we have an issue with our release and this will improve that process. |
I'd like to take another look at this - I'm not a fan of adding code to the plugin that's only there to support functional tests. Also, the issue raised in the PR description:
Should have been solved by #6424. Agree with the goal of not using Maven local repo at all, but I don't think this should be merged. |
@3flex I'm unsure this solved it. It's easy to verify by just bumping the I'm up for any alternative that makes use not fallback to Maven Central or Maven Local for running our tests as this overcomplicates the release process |
Our tests should not depend on
mavenLocal()
at all.The problem is that
detekt-gradle-plugin
is applying a dependency ondetekt-cli
using the version defined here:detekt/build-logic/src/main/kotlin/Versions.kt
Line 5 in 2eccd64
So whenever that version gets bumped, test starts to fail unless you
publishToMavenLocal
as they'll try to find the next version on Maven Central, which won't be there.That's also the reason why sometimes those PRs:
fail on CI as they can't find the newer artifact for
detekt-cli
on Maven Central yet.This is blocking any automation of the release process essentially, and we should move away from it. See:
In this PR I'm moving away from using the project version to apply the
detekt-cli
to use the latest published stable. This allows those tests to rely just onmavenCentral()
.The dependency will be updated by Renovate bot as we release stable versions (we're doing the same thing for the formatting of the
detekt-gradle-plugin
itself).