GradleEnterprise -> Develocity #27

blindpirate · 2024-04-23T05:03:13Z

Fixes #26

Upgrades to develocity plugin and eliminate all deprecated API usages. Tests are updated accordingly.

build.gradle.kts

cobexer · 2024-04-23T06:12:43Z

build.gradle.kts

@@ -45,9 +45,9 @@ extensions.configure<ExtraPropertiesExtension>("ext") {
 gradlePlugin {
    plugins.create("conventionsPlugin") {
        id = "io.github.gradle.gradle-enterprise-conventions-plugin"


Should we remove gradle-enterprise from the plugin ID here?

cobexer · 2024-04-23T06:25:39Z

build.gradle.kts

-        implementationClass = "com.gradle.enterprise.conventions.GradleEnterpriseConventionsPlugin"
-        displayName = "Gradle Enterprise Conventions Plugin"
-        description = "Gradle Enterprise Conventions Plugin"
+        implementationClass = "com.gradle.enterprise.conventions.DevelocityConventionsPlugin"


Should we move the relevant classes to the io.github.gradle package and remove the enterprise part while we are at it?

We should also consider JEP 261 and make an effort to avoid the package name collision risk with develocity itself

I intentionally keep all "interface" of this plugin unchanged (like package name, implementation class name, etc.). Let's do that in a separate PR.

I don't see how a separate PR is better than separate commits in this PR?

Renaming the implementation class means moving all files to another directory, which in some cases confuses GitHub web UI to display the diffs - showing all files as "new". That's why I want to do the moving in a separate PR - to make this PR show "diffs".

cobexer · 2024-04-23T06:33:16Z

src/main/java/com/gradle/enterprise/conventions/customvalueprovider/DevelocityConventions.java

-public class GradleEnterpriseConventions {
-    private static final Logger LOGGER = Logging.getLogger(GradleEnterpriseConventions.class);
+public class DevelocityConventions {
+    private static final Logger LOGGER = Logging.getLogger(DevelocityConventions.class);
    private static final String DEFAULT_GRADLE_ENTERPRISE_SERVER = "https://ge.gradle.org";


DEFAULT_GRADLE_ENTERPRISE_SERVER => DEFAULT_DEVELOCITY_SERVER?

cobexer · 2024-04-23T06:34:18Z

src/main/java/com/gradle/enterprise/conventions/customvalueprovider/DevelocityConventions.java

-public class GradleEnterpriseConventions {
-    private static final Logger LOGGER = Logging.getLogger(GradleEnterpriseConventions.class);
+public class DevelocityConventions {
+    private static final Logger LOGGER = Logging.getLogger(DevelocityConventions.class);
    private static final String DEFAULT_GRADLE_ENTERPRISE_SERVER = "https://ge.gradle.org";
    private static final String AGREE_PUBLIC_BUILD_SCAN_TERM_OF_SERVICE = "agreePublicBuildScanTermOfService";
    private static final String GRADLE_ENTERPRISE_URL_PROPERTY_NAME = "gradle.enterprise.url";


Also add a (prioritized) lookup for gradle.develocity.url or develocity.url?

Done here: 56329e0

wolfs · 2024-04-23T06:45:25Z

build.gradle.kts

-        displayName = "Gradle Enterprise Conventions Plugin"
-        description = "Gradle Enterprise Conventions Plugin"
+        implementationClass = "com.gradle.enterprise.conventions.DevelocityConventionsPlugin"
+        displayName = "Develocity Conventions Plugin"


Is this published to the plugin portal? Then we should make clear that this is a Develocity conventions plugin for the Gradle Github organization.

Yes it is: https://plugins.gradle.org/plugin/io.github.gradle.gradle-enterprise-conventions-plugin

Let's fix this in another PR.

Again, what is the benefit of a separate PR?

Changing the implementation class means moving all files to another directory. I don't like to mix them as that would mess up file diffs.

wolfs · 2024-04-23T06:46:41Z

src/main/java/com/gradle/enterprise/conventions/DevelocityConventionsPlugin.java

@@ -1,17 +1,15 @@
 package com.gradle.enterprise.conventions;


🤔 I don't think the package com.gradle.enterprise is great. I don't think it should be com.gradle at all since it is for the open source parts of Gradle. Let's rename those things in a separate PR.

Yes, let's do this in separate PR. This PR only switch from gradle enterprise API to develocity API.

wolfs · 2024-04-23T06:48:34Z

src/main/java/com/gradle/enterprise/conventions/PublishingConfigurationAction.java

+    public static final PublishingConfigurationAction PUBLISH_IF_AUTHENTICATED = new PublishingConfigurationAction("publishWhenAuthenticated") {
+        @Override
+        public void execute(BuildScanPublishingConfiguration publishing) {
+            publishing.onlyIf(BuildScanPublishingConfiguration.PublishingContext::isAuthenticated);


🤔 Could we be a little bit better here now? I'd say we publish if

We are authenticated

or someone requests --scan via the command line.

WDYT?

That would mean changing the behavior of this param:

gradle-org-conventions-plugin/src/main/java/com/gradle/enterprise/conventions/DevelocityConventionsPlugin.java

Line 90 in f2d63e9

String strategy = conventions.getSystemProperty("publishStrategy", "publishAlways");

We could do that but I'd keep this PR only deal with renaming.

wolfs · 2024-04-23T06:50:07Z

src/test/java/com/gradle/enterprise/fixtures/ProxyProperty.java

+import java.io.IOException;
+import java.util.function.BiFunction;
+
+abstract class ProxyProperty<T> implements Property<T> {


🤔 Why do we need this for testing? Can we get rid of it now?

Plz see the answer in my next comment. It's used to match the develocity plugin interface and adapt to serialization/deserialization.

wolfs · 2024-04-23T06:51:12Z

src/test/java/com/gradle/enterprise/fixtures/DevelocityConfigurationForTest.java

+
+import javax.annotation.Nullable;
+
+public class DevelocityConfigurationForTest implements DevelocityConfiguration {


🤔 Can't we use a regular mocking framework like Mockito or Spock?

The integration test we do right now is:

Create a "fake" develocity plugin with same id/implementation class.

Run the Gradle build with the fake develocity plugin via TestKit. The fake develocity plugin accepts configuration actions from the build and stores the configurations in this DevelocityConfigurationForTest class.

At the end of Gradle build, the configurations are written to a JSON file.

In the integration test, the JSON file is deserialized to verify the configurations are correct.

So in the integration test, we need to pass information between the "outside" Gradle build (the integration test JVM) and the "inside" Gradle build (the build running with fake develocity plugin) - we can't create a mockito mock in JVM1 and use it in JVM2. Plus, we need to serialize/deserialize the configuration, this DevelocityConfigurationForTest is also used as data class.

Is there a better way to test this? And even if we use a fake develocity plugin, why can't the fake develocity plugin use real Property instances instead of this strange ProxyProperty?

PLMK if you have better idea, but in this PR let's keep this imperfect way. I pushed a new commit to use a real Property.

blindpirate added 2 commits April 23, 2024 13:02

GradleEnterprise -> Develocity

11f64e2

GradleEnterprise -> Develocity

f2d63e9

blindpirate force-pushed the bz/fix-deprecated-api branch from 11f64e2 to f2d63e9 Compare April 23, 2024 05:53

cobexer requested changes Apr 23, 2024

View reviewed changes

wolfs reviewed Apr 23, 2024

View reviewed changes

blindpirate added 5 commits April 26, 2024 14:09

Resolve review feedback

56329e0

Merge branch 'blindpirate/fix-deprecated-api' into bz/fix-deprecated-api

7f181b5

Fix wrong conflict resolution

052a054

Remove ProxyProperty

a1f2c7a

Remove more ProxyProperty

fd047d8

blindpirate requested review from cobexer and wolfs May 8, 2024 01:32

cobexer approved these changes May 8, 2024

View reviewed changes

blindpirate merged commit ffcc47f into master May 9, 2024
3 checks passed

blindpirate deleted the bz/fix-deprecated-api branch May 9, 2024 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GradleEnterprise -> Develocity #27

GradleEnterprise -> Develocity #27

blindpirate commented Apr 23, 2024 •

edited

cobexer Apr 23, 2024

cobexer Apr 23, 2024

blindpirate Apr 26, 2024

cobexer Apr 30, 2024

blindpirate May 1, 2024

cobexer Apr 23, 2024

cobexer Apr 23, 2024

blindpirate Apr 26, 2024

wolfs Apr 23, 2024

cobexer Apr 23, 2024

blindpirate Apr 26, 2024

cobexer Apr 30, 2024

blindpirate May 8, 2024

wolfs Apr 23, 2024

blindpirate Apr 26, 2024

wolfs Apr 23, 2024

blindpirate Apr 26, 2024

wolfs Apr 23, 2024

blindpirate Apr 26, 2024 •

edited

wolfs Apr 23, 2024

blindpirate Apr 26, 2024 •

edited

wolfs Apr 26, 2024

blindpirate Apr 26, 2024


		import javax.annotation.Nullable;

		public class DevelocityConfigurationForTest implements DevelocityConfiguration {

GradleEnterprise -> Develocity #27

GradleEnterprise -> Develocity #27

Conversation

blindpirate commented Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blindpirate Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blindpirate Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blindpirate commented Apr 23, 2024 •

edited

blindpirate Apr 26, 2024 •

edited

blindpirate Apr 26, 2024 •

edited