#391 Update pipeline step messaging to be compatible with Java 17 #407
Conversation
| <plugin> |
There was a problem hiding this comment.
I: Fixed tabbing issue in downstream project poms where this whole block was not being indented
| @@ -58,9 +59,10 @@ public final class CdiContainerFactory { | |||
| protected static List<CdiContext> getContexts() { | |||
| List<CdiContext> contexts = new ArrayList<>(); | |||
| contexts.add(new PipelinesCdiContext()); | |||
| ## TODO: would be nice to only include if we need messaging, but right now we assume messaging everywhere | |||
| ## TODO: would be nice to only include if we need messaging, but right now we assume kafka messaging everywhere | |||
There was a problem hiding this comment.
I: Space before ## TODO was being rendered in downstream template, resulting in the spacing being off in the following line.
|
|
||
| import io.smallrye.config.inject.ConfigExtension; |
There was a problem hiding this comment.
I: Removed unused import
| @@ -3,11 +3,8 @@ package ${basePackage}.cdi; | |||
| import java.util.List; | |||
|
|
|||
| import jakarta.enterprise.inject.spi.Extension; | |||
| import com.boozallen.aissemble.core.filestore.EnvironmentVariableFileStoreConfig; | |||
There was a problem hiding this comment.
I: Removed unused import
|
|
||
| protected String indent; | ||
|
|
||
| protected void detectAndSetIndent(File file) { |
There was a problem hiding this comment.
A: Probably a candidate for Baton's PomHelper. Do I recognize this from one of the patch release branches? If so, I might already have a branch locally to add this to Baton.
There was a problem hiding this comment.
Yes I copied this from an old branch. Guess it never got added to dev after a patch release.
| <dependency> | ||
| <groupId>com.boozallen.aissemble</groupId> | ||
| <artifactId>extensions-messaging-kafka</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Q: should this be a default inclusion for all spark pipeline?
There was a problem hiding this comment.
Yes. Unfortunately all our pipelines include:
<dependency>
<groupId>io.smallrye.reactive</groupId>
<artifactId>smallrye-reactive-messaging-in-memory</artifactId>
<version>${version.smallrye.reactive.messaging}</version>
<scope>test</scope>
</dependency>
by default. So in order to remain compatible with that, we must include the new extensions-messaging-kafka by default.
| When the Spark Pipeline Messaging migration executes | ||
| Then the aissemble kafka messaging dependency is added to the POM | ||
| And the CDI container factory is updated with the messaging and kafka context objects | ||
| And the pipeline CDI context has the unecessary Kafka beans and imports removed |
There was a problem hiding this comment.
S: I'd skip this bit. It's just cleanup, and there's a chance this could cause a compile error that would be difficult to get out of without just disabling the migration. And if you had to disable it on the first pipeline in your project, subsequent pipelines wouldn't get the automated migration.
There was a problem hiding this comment.
Verified this will not cause any issues leaving the extra beans
carter-cundiff
force-pushed
the
391-messaging-java-17
branch
2 times, most recently
from
f745adf
to
f53bb38
Compare
| boolean cdiContainerFactoryFileMigrationSuccess = false; | ||
| if (shouldMigrateCdiContainerFactoryFile(cdiContainerFactoryFile, addKafkaCdiContext)) { | ||
| // Update the file with the new import(s) and CDI context object(s) | ||
| cdiContainerFactoryFileMigrationSuccess = this.migrateCdiContainerFactoryFile(cdiContainerFactoryFile, addKafkaCdiContext); |
There was a problem hiding this comment.
S/I: We should avoid modifying files that aren't directly matched by Baton as much as possible. Baton creates back ups of files and can roll them back -- but that logic breaks when you violate the contract with Baton of only modifying files that your migration directly matched.
One way we could have done this would be to have a separate migration only for CdiContainerFactory.java that essentially does a check like:
this.hasDependency(this.getMavenProject(),
SMALLRYE_REACTIVE_MESSAGING_GROUP_ID,
SMALLRYE_REACTIVE_MESSAGING_KAFKA_ARTIFACT_ID)
&& shouldMigrateCdiContainerFactoryFile(file);There was a problem hiding this comment.
FWIW, I think this is probably OK for a one off, as long as we avoid it in the future.
There was a problem hiding this comment.
Ah don't think I realized you could access the maven project from a non-pom migration, will update!
carter-cundiff
force-pushed
the
391-messaging-java-17
branch
from
f53bb38
to
4ad021c
Compare
carter-cundiff
force-pushed
the
391-messaging-java-17
branch
from
4ad021c
to
ad25749
Compare
No description provided.