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

Use Action instead of Kotlin lambda. #5

Merged
merged 2 commits into from
Apr 24, 2022

Conversation

autonomousapps
Copy link
Contributor

@autonomousapps autonomousapps commented Apr 23, 2022

Two commits:

  1. First uses Gradle's Action interface for better DSL support.
  2. Adds an initial gradleTest setup for TestKit tests.

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2022

CLA assistant check
All committers have signed the CLA.

@autonomousapps autonomousapps marked this pull request as ready for review April 23, 2022 19:24
@handstandsam
Copy link
Collaborator

This is awesome! Thanks for fixing the Groovy syntax issues with this!

Do we need another Gradle wrapper? Can't we run tasks from the root? : dependency-guard-gradle-plugin ?

If we do need the wrapper can we use symlinks to the root gradlew and other wrapper files?

@@ -0,0 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-bin.zip
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we need it, symlinks would keep this on 7.4.2 like the other.

@handstandsam
Copy link
Collaborator

Will do a proper look over tomorrow after I am back on a computer and not on my phone.

Thanks again!

@autonomousapps
Copy link
Contributor Author

Failing with an API check (predictably):

> API check failed for project dependency-guard-gradle-plugin.
  --- /home/runner/work/dependency-guard/dependency-guard/dependency-guard-gradle-plugin/api/dependency-guard-gradle-plugin.api
  +++ /home/runner/work/dependency-guard/dependency-guard/dependency-guard-gradle-plugin/build/api/dependency-guard-gradle-plugin.api
  @@ -21,7 +21,7 @@
   public class com/dropbox/gradle/plugins/dependencyguard/DependencyGuardPluginExtension {
   	public fun <init> ()V
   	public final fun configuration (Ljava/lang/String;)V
  -	public final fun configuration (Ljava/lang/String;Lkotlin/jvm/functions/Function1;)V
  +	public final fun configuration (Ljava/lang/String;Lorg/gradle/api/Action;)V
   }

@autonomousapps
Copy link
Contributor Author

This is awesome! Thanks for fixing the Groovy syntax issues with this!

Do we need another Gradle wrapper? Can't we run tasks from the root? : dependency-guard-gradle-plugin ?

If we do need the wrapper can we use symlinks to the root gradlew and other wrapper files?

I wouldn't recommend symlinks because they won't work on Windows.

I didn't even try to create the wrapper. It just sort of happened. In retrospect, I'm not sure how. Maybe when I imported it into IDEA?

The reason I was running tasks from the plugin directory was because that's where the settings.gradle.kts file was; this is a marker for a root project.

}
plugins {
id 'com.gradle.enterprise' version '3.10'
//id 'com.dropbox.dependency-guard' version '$pluginVersion'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it should have been deleted. Because this PR uses withPluginClasspath(), we don't need this. Just something I was playing with originally and forgot to fully delete.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I'll delete this!

mavenCentral()
}
plugins {
id 'com.gradle.enterprise' version '3.10'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most people don't have gradle enterprise, do we need this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That actually publishes the tests to the public scans server. But no, it's not necessary. It is sometimes useful for debugging why a test fails by looking at the build scan for it.

@handstandsam
Copy link
Collaborator

Merging for progress and since this is still a snapshot. I'll clean up the tests in CI and link to that in a few.

@handstandsam handstandsam merged commit bbd7bb9 into dropbox:main Apr 24, 2022
@handstandsam
Copy link
Collaborator

Removed the extra gradle wrapper here: #6

Will just push the removal of the comment to main.

@autonomousapps autonomousapps deleted the action-and-tests branch April 25, 2022 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants