Skip to content

Add support for generating code to send MuleSoft Dataweave Transformations to TriggerMesh #183

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

Merged
merged 11 commits into from
Sep 23, 2022

Conversation

cab105
Copy link
Contributor

@cab105 cab105 commented Jun 30, 2022

I introduced a new runtime flag for spring-boot-migrator called sbm.muleTriggerMeshTransformEnabled that will modify the generated mule-to-boot recipe for handling DataWeave transformations to stream the transformation to TriggerMesh's Dataweave Transformer.

The generated code works best when the service runs inside a Kubernetes cluster leveraging Knative's Sink Binding to associate the the newly migrated service to the TriggerMesh Dataweave transformation, however the service can run on it's own by setting the K_SINK environment variable to the exposed transformation service.

I created a gist to demonstrate what an integration within TriggerMesh would look like.

@fabapp2
Copy link
Contributor

fabapp2 commented Jul 7, 2022

@cab105 Thank you for your contribution!

I had three comments. Feel free to ping me if you want to discuss in person?

@cab105
Copy link
Contributor Author

cab105 commented Jul 7, 2022

@cab105 Thank you for your contribution!

I had three comments. Feel free to ping me if you want to discuss in person?

Thank you for taking a look at @fabapp2 . Will address the rest template, tabs, and test issues, then resubmit.

@sanagaraj-pivotal
Copy link
Contributor

Could we get an integration test for this feature, please refer this test for info

@cab105 cab105 force-pushed the tm-transformation branch from f265415 to da0fef5 Compare July 17, 2022 03:10
@cab105 cab105 force-pushed the tm-transformation branch 2 times, most recently from 711359f to 78f2ee8 Compare August 7, 2022 22:20
@fabapp2
Copy link
Contributor

fabapp2 commented Aug 15, 2022

Hi @cab105

Are you facing issues with this? If you like we can pair to get the last few bits added and this PR merged?

@cab105
Copy link
Contributor Author

cab105 commented Aug 16, 2022

The codebase has been pretty unstable due to the springboot 3 work, but does look like it has now solidified, and am identifying issues with my original test code that need to be addressed. Once that is resolved, then we should be good to go.

@cab105 cab105 force-pushed the tm-transformation branch from a376ef0 to 31c1533 Compare August 25, 2022 02:23
@cab105
Copy link
Contributor Author

cab105 commented Aug 25, 2022

Resync from upstream main branch, added unit tests, and migrated the primary dataweave transformation into a template file.

@cab105
Copy link
Contributor Author

cab105 commented Sep 6, 2022

So I've been able to resolve some of my outstanding issues, and have fixed a couple of issues in SBM proper (#385 and #373). Given the current Mule integration test that was cited in the comments is disabled due to #351, I'm wondering if there's anything else that would be required for this PR (save for a resync with upstream)?

import static org.assertj.core.api.Assertions.assertThat;

@TestMethodOrder(MethodOrderer.MethodName.class)
//@Disabled("Temporary disabled before CI will be fixed with docker in docker issue: #351")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the base test, this annotation is in effect, but left it commented out to facilitate verification for review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I get you here, but the method order is obsolete as there's only one test and no shared state, or?
You can remove the commented @Disabled, the test is green 🚀

IntegrationTestBaseClass.beforeAll();

// Will need to ensure this is set globally for the test
System.setProperty("sbm.muleTriggerMeshTransformEnabled", "true");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into overriding @SpringProperties, but this would not get picked up by the transformer that would swap out the stub code for the code that will send the transformation out to the TriggerMesh Dataweave transformer. If there's a cleaner way to do this, then would appreciate hearing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rework this approach and set the flag in the action of a dedicated recipe, e.g. migrate-mule-to-boot-tm and then just call it from here.

@@ -60,13 +91,19 @@ public DslSnippet translate(
String flowName,
Map<Class, MuleComponentToSpringIntegrationDslTranslator> translatorsMap
) {
// Ugly hack to work around an inability to inject a sbm property into the mulesoft parser.
String isTmTransformationEnabled = System.getProperty("sbm.muleTriggerMeshTransformEnabled");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to work around case when running unit test and @Autowired is unable to bring in the SBM properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically speaking, this should work

@Autowired
private SbmApplicationProperties p;

public void m() {
   p.getPropertyXyz();
}

But in this case an annotation on type level @ConditionalOnProperty(name = "sbm.muleTriggerMeshTransformEnabled", havingValue = "true") of a dedicated implementation would would be better.

Copy link
Contributor

@fabapp2 fabapp2 left a comment

Choose a reason for hiding this comment

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

Hi @cab105 !
Thanks for your effort! 🚀

I think the hacky ;) bits will disapear as soon as the application property is removed and the action gets parametrized per recipe, which feels somehow naturally (now).
Everything else lgtm!

IntegrationTestBaseClass.beforeAll();

// Will need to ensure this is set globally for the test
System.setProperty("sbm.muleTriggerMeshTransformEnabled", "true");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rework this approach and set the flag in the action of a dedicated recipe, e.g. migrate-mule-to-boot-tm and then just call it from here.

String description = "Migrate Mulesoft 3.9 to Spring Boot.";

// Flag to enable TriggerMesh ransformation mode
if (sbmProperties.isMuleTriggerMeshTransformEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should better create two bean methods here, calling one factory method that creates the recipe twice with the flag true and false. Taht would then finally allow to remove the application property :)

Copy link
Contributor

@fabapp2 fabapp2 left a comment

Choose a reason for hiding this comment

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

I just realize that there must be some way to toggle these translators if boot creates them...
but it should not be an application/system property when possible...
it should come from the recipe/action...
this could be tricky...

@@ -60,13 +91,19 @@ public DslSnippet translate(
String flowName,
Map<Class, MuleComponentToSpringIntegrationDslTranslator> translatorsMap
) {
// Ugly hack to work around an inability to inject a sbm property into the mulesoft parser.
String isTmTransformationEnabled = System.getProperty("sbm.muleTriggerMeshTransformEnabled");
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically speaking, this should work

@Autowired
private SbmApplicationProperties p;

public void m() {
   p.getPropertyXyz();
}

But in this case an annotation on type level @ConditionalOnProperty(name = "sbm.muleTriggerMeshTransformEnabled", havingValue = "true") of a dedicated implementation would would be better.

@cab105
Copy link
Contributor Author

cab105 commented Sep 21, 2022

I just realize that there must be some way to toggle these translators if boot creates them...
but it should not be an application/system property when possible...
it should come from the recipe/action...
this could be tricky...

It is. I originally used your approach of @Autowired on SbmApplicationProperties, but the unit test doesn't wire in the SbmApplicationProperties resulting in the tests to use the default behavior, and why I explicitly checked/set the System properties where I did. Granted if there was an easier way to pass a flag through the rewrite engine, then I'd be happy, but it appeared to require a lot more refactoring than I felt comfortable to pass the single flag down to the DwlTransformTranslator class.

@fabapp2
Copy link
Contributor

fabapp2 commented Sep 22, 2022

I just realize that there must be some way to toggle these translators if boot creates them...
but it should not be an application/system property when possible...
it should come from the recipe/action...
this could be tricky...

It is. I originally used your approach of @Autowired on SbmApplicationProperties, but the unit test doesn't wire in the SbmApplicationProperties resulting in the tests to use the default behavior, and why I explicitly checked/set the System properties where I did. Granted if there was an easier way to pass a flag through the rewrite engine, then I'd be happy, but it appeared to require a lot more refactoring than I felt comfortable to pass the single flag down to the DwlTransformTranslator class.

The problem is that the test is not a @SpringBootTest and the SbmApplicationProperties would need to be added to the TestProjectContext test harness. But due to the inheritance design it is not (yet) possible.

What about...
you leave the property as it is, for now, fix the few comments left and we finally merge this back in?
And I will think about a way to remove the need for the application property.
Not 💯 sure yet how this will look like though...

@fabapp2 fabapp2 added the type: enhancement New feature or request label Sep 23, 2022
@fabapp2 fabapp2 added this to the v0.12.0 milestone Sep 23, 2022
@fabapp2 fabapp2 linked an issue Sep 23, 2022 that may be closed by this pull request
@fabapp2 fabapp2 merged commit caab9e0 into spring-projects-experimental:main Sep 23, 2022
@cab105 cab105 deleted the tm-transformation branch September 23, 2022 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supports TriggerMesh for MuleSoft Dataweave Transformations
3 participants