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

[5.x] Add support for WildFly Glow #374

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Conversation

jfdenise
Copy link
Contributor

@jfdenise jfdenise commented Sep 5, 2023

No description provided.

@jfdenise jfdenise requested a review from jamezp as a code owner September 5, 2023 14:57
Copy link
Member

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

Just a couple minor changes/comments.

}
if (results.getErrorSession().hasErrors()) {
if (discoverProvisioningInfo.isFailsOnError()) {
throw new MojoExecutionException("Error detected by glow. Aborting.");
Copy link
Member

Choose a reason for hiding this comment

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

Will errors be printed somewhere? What I mean by this, is will glow log some kind of error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, glow has a pluggable message writer tha , in the plugin cases route logs to maven logs: https://github.com/jfdenise/wildfly-maven-plugin/blob/glow-only/plugin/src/main/java/org/wildfly/plugin/common/GlowMavenMessageWriter.java
BTW I am changing glow by "WildFly Glow"

@@ -195,6 +202,9 @@ public void execute() throws MojoExecutionException, MojoFailureException {
getLog().debug(String.format("Skipping " + getGoal() + " of %s:%s", project.getGroupId(), project.getArtifactId()));
return;
}
if (dryRun) {
getLog().info("Dry run execution, no server provisioned.");
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but I think we should say something like "Dry run execution, no server will be provisioned"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks.

Path targetPath = Paths.get(project.getBuild().getDirectory());
Path file = targetPath.resolve(PLUGIN_PROVISIONING_FILE);
getLog().info("Dry-run execution, generating provisioning.xml file: " + file);
try (FileWriter writer = new FileWriter(file.toFile())) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should use something like:

try (BufferedWritter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks.

Comment on lines 53 to 54
List<Path> lst = new ArrayList<>();
lst.add(deployment);
Copy link
Member

Choose a reason for hiding this comment

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

If this is the only entry, we should just use List.of(deployment);.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks.

@jfdenise
Copy link
Contributor Author

jfdenise commented Sep 6, 2023

Failure is unrelated to this change. Known instability when producing the docker image.

Copy link
Member

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

Just one more minor thing. I would think we need to undeploy, for exploded content, before we delete the content.

Comment on lines 748 to 762
// The first packaging was not an exploded war, clean it.
if (requiresWarDeletion) {
final Path path = resolveWarLocation();
deleteRecursively(path);
requiresWarDeletion = false;
repackaged = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

We should likely perform an undeploy, at least if it's not remote, before we delete the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamezp , thank-you. I found some issues with the remote goal that I fixed. When remote is enabled, this logic shouldn't be applied and the reprovisioning/re-start shouldn't occur. I also found an issue with the directory name used by the war plugin when the "war" goal is used. In this case, the directory can't contain the .war extension. This extension has been added for Glow to understand what kind of exploded deployment we are dealing with. In the remote case, Glow scanning is meaningless.

@jamezp
Copy link
Member

jamezp commented Sep 6, 2023

I've also bumped main to 5.0.0.Alpha1-SNAPSHOT

@jfdenise
Copy link
Contributor Author

jfdenise commented Sep 7, 2023

Failing test is un-related to this change.

@jamezp jamezp merged commit 9352aeb into wildfly:main Sep 8, 2023
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.

2 participants