Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Remove usage of Project during task execution #455

Closed
wants to merge 2 commits into from
Closed

Remove usage of Project during task execution #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e26e26
Remove usage of Project during task execution
liutikas Mar 1, 2023
198c45e
Merge branch 'master' into projectduringexec
diegomarquezp Jul 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/StageAppYamlTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,17 @@

import com.google.cloud.tools.appengine.AppEngineException;
import com.google.cloud.tools.appengine.operations.AppYamlProjectStaging;
import javax.inject.Inject;
import org.gradle.api.DefaultTask;
import org.gradle.api.internal.file.FileOperations;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not import org.gradle.api.internal packages. Those are not meant for general use in plugin code

Suggested change
import org.gradle.api.internal.file.FileOperations;
import org.gradle.api.file.FileSystemOperations;

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not possible to use the public API here, because it was added in Gradle 6.0 as you said.

import org.gradle.api.tasks.Nested;
import org.gradle.api.tasks.TaskAction;

/** Stage App Engine app.yaml based applications for deployment. */
public class StageAppYamlTask extends DefaultTask {
public abstract class StageAppYamlTask extends DefaultTask {

@Inject
abstract public FileOperations getFileOperations();
Copy link
Contributor

Choose a reason for hiding this comment

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

FileSystemOperations requires Gradle 6.0+
Recently there was a commit comparing the version with 5.5 (see #451 ), so I'm not sure if bumping the minimal version to Gradle 6.0 is generally expected from an innocent change like this

Suggested change
abstract public FileOperations getFileOperations();
abstract public FileSystemOperations getFileSystemOperations();

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't take my usage of 5.1 as a guide :) I used the lowest possible point where the new API is available. During maintenance this makes the condition disappear faster than if it's at the point of deprecation.

A more relevant discussion might be: #450

Copy link
Contributor

@TWiStErRob TWiStErRob Aug 4, 2023 •
edited

Choose a reason for hiding this comment

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

So the minimum supported version is Gradle 4.0, which means this suggestion can't work.

I think the getFileOperations() needs a

// TODO use public org.gradle.api.file.FileSystemOperations once the minimum version is greater than Gradle 6.0.

but until then it's safe to use as the FileOperations API, however internal, was added in Gradle 0.9 and hasn't changed since then (for this use case).

Alternatively we could use some other library to do the same. e.g.

org.gradle.util.GFileUtils.deleteDirectory(appYamlExtension.getStagingDirectory());

or

org.apache.tools.ant.util.FileUtils.delete(appYamlExtension.getStagingDirectory());

or plain old Java 8 (assuming minimum supported version allows, what is the minimum Java for this plugin? Gradle 4.0 supports Java 7 minimum):

Files.walk(pathToBeDeleted)
      .sorted(Comparator.reverseOrder())
      .map(Path::toFile)
      .forEach(File::delete);

and since the .delete() just deleted the folder it's possible to just use plain appYamlExtension.getStagingDirectory().mkdir().

Copy link
Contributor

Choose a reason for hiding this comment

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

gradlew googleJavaFormat wants this modifier order:

Suggested change
abstract public FileOperations getFileOperations();
public abstract FileOperations getFileOperations();


private StageAppYamlExtension appYamlExtension;

Expand All @@ -40,8 +45,8 @@ public void setStagingConfig(StageAppYamlExtension stagingConfig) {
/** Task entrypoint : Stage the app.yaml based application. */
@TaskAction
public void stageAction() throws AppEngineException {
getProject().delete(appYamlExtension.getStagingDirectory());
getProject().mkdir(appYamlExtension.getStagingDirectory().getAbsolutePath());
getFileOperations().delete(appYamlExtension.getStagingDirectory());
getFileOperations().mkdir(appYamlExtension.getStagingDirectory().getAbsolutePath());

AppYamlProjectStaging staging = new AppYamlProjectStaging();
staging.stageArchive(appYamlExtension.toAppYamlProjectStageConfiguration());
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/cloud/tools/gradle/appengine/core/DownloadCloudSdkTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public void downloadCloudSdkAction()
}

ProgressListener progressListener = new NoOpProgressListener();
ConsoleListener consoleListener = new DownloadCloudSdkTaskConsoleListener(getProject());
ConsoleListener consoleListener = new DownloadCloudSdkTaskConsoleListener(getLogger());

// Install sdk if not installed
if (!managedCloudSdk.isInstalled()) {
Expand Down
9 changes: 5 additions & 4 deletions ...ava/com/google/cloud/tools/gradle/appengine/core/DownloadCloudSdkTaskConsoleListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@
import com.google.cloud.tools.managedcloudsdk.ConsoleListener;
import org.gradle.api.Project;
Copy link
Contributor

Choose a reason for hiding this comment

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

gradlew googleJavaFormat found an unused import here, which makes sense since you removed the field.

import org.gradle.api.logging.LogLevel;
import org.gradle.api.logging.Logger;

public class DownloadCloudSdkTaskConsoleListener implements ConsoleListener {
private Project project;
private Logger logger;

public DownloadCloudSdkTaskConsoleListener(Project project) {
this.project = project;
public DownloadCloudSdkTaskConsoleListener(Logger logger) {
this.logger = logger;
}

@Override
Expand All @@ -36,7 +37,7 @@ public void console(String rawString) {
// is that Gradle redirects standard output to its logging system at the QUIET level. So, in
// order to print to LIFECYCLE without adding a newline, we just check that our desired level
// is enabled before trying to print.
if (project.getLogger().isEnabled(LogLevel.LIFECYCLE)) {
if (logger.isEnabled(LogLevel.LIFECYCLE)) {
System.out.print(rawString);
}
}
Expand Down