Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Multi route support #638

Merged
merged 4 commits into from
May 8, 2023
Merged

Multi route support #638

merged 4 commits into from
May 8, 2023

Conversation

Delawen
Copy link
Member

@Delawen Delawen commented May 5, 2023

Backend has now an experimental v2 api that supports multiroutes. Not all endpoints are ported from v1 yet.

Integration kind is not fully supported yet. This PR focused on Camel Routes, Kamelets, and Bindings.
Mixing different DSLs on the same file is not supported.

Superseeds #633

lordrip and others added 2 commits May 3, 2023 13:19
Integration kind CRD is still not fully supported.
Mixing different DSLs on the same file is not supported.
@Delawen Delawen requested a review from a team May 5, 2023 10:06
@Delawen
Copy link
Member Author

Delawen commented May 5, 2023

@lordrip 👀

api/pom.xml Show resolved Hide resolved
@Consumes(MediaType.APPLICATION_JSON)
@Produces("text/yaml")
@Path("/")
@CacheResult(cacheName = "api")
Copy link
Member Author

Choose a reason for hiding this comment

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

Caching (see above comment)

Copy link
Member

Choose a reason for hiding this comment

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

How would this cache behave? Is this the correct "doc" for this annotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly that one.

final @Parameter(description = "DSL to use. For example: "
+ "'Kamelet Binding'.")
@QueryParam("dsl") String dsl) {
List<Integration> integrations = new ArrayList<>();
Copy link
Member Author

Choose a reason for hiding this comment

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

We should move all this to some service and keep the resources classes clean, but better not add more changes, it is already very long.

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #638 (a389ebd) into main (4c347fc) will decrease coverage by 0.96%.
The diff coverage is 67.09%.

@@             Coverage Diff              @@
##               main     #638      +/-   ##
============================================
- Coverage     78.54%   77.58%   -0.96%     
- Complexity      132      146      +14     
============================================
  Files            45       46       +1     
  Lines          2358     2503     +145     
  Branches        370      393      +23     
============================================
+ Hits           1852     1942      +90     
- Misses          339      384      +45     
- Partials        167      177      +10     
Impacted Files Coverage Δ
...arser/camelroute/IntegrationStepParserService.java 73.17% <0.00%> (-10.17%) ⬇️
...ployment/generator/DeploymentGeneratorService.java 100.00% <ø> (ø)
...end/api/service/step/parser/StepParserService.java 50.00% <ø> (ø)
...elroute/IntegrationDeploymentGeneratorService.java 50.61% <10.00%> (-6.33%) ⬇️
...kend/api/service/deployment/DeploymentService.java 51.61% <48.00%> (-2.45%) ⬇️
...melroute/CamelRouteDeploymentGeneratorService.java 75.00% <66.66%> (-1.93%) ⬇️
...ployment/generator/kamelet/KameletRepresenter.java 82.14% <70.58%> (-3.29%) ⬇️
.../backend/api/resource/v2/IntegrationsResource.java 73.91% <73.91%> (ø)
...elet/KameletBindingDeploymentGeneratorService.java 67.79% <81.25%> (+1.77%) ⬆️
...tor/kamelet/KameletDeploymentGeneratorService.java 69.69% <83.33%> (+2.42%) ⬆️
... and 4 more

@@ -0,0 +1,393 @@
package io.kaoto.backend.api.resource.v2;
Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the v1 file here thinking at some point we will deprecate and remove v1 and its tests.

At the end the round trip function.

var to = (Map<String, Object>) ((Map<String, Object>) steps.get(0)).get("to");
assertEquals("log:", to.get("uri"));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fun part for this PR


assertEquals(res0.extract().body().asString(), res1.extract().body().asString());

var flows = Arrays.asList(res1.extract().body().as(Integration[].class));
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that now we return a list/array of Integrations.

@@ -0,0 +1,2798 @@
[
Copy link
Member Author

Choose a reason for hiding this comment

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

JSON responses now are arrays/lists, not single objects. So I had to copy some resources files and added the multi string to distinguish.

uploadMode: '{{uploadMode}}'
accessToken: '{{accessToken}}'

---
Copy link
Member Author

Choose a reason for hiding this comment

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

😈 multi Kamelets on the same file MUAHAHAHA

@@ -107,6 +121,12 @@ public boolean appliesTo(final List<Step> steps) {
.anyMatch(Predicate.isEqual(s.getKind().toUpperCase())));
}

@Override
public boolean appliesToFlows(List<StepParserService.ParseResult<Step>> flows) {
Copy link
Member Author

Choose a reason for hiding this comment

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

If one of the flows in the list of flows is from this DSL we assume that we can handle it.

@Override
public List<ParseResult<Step>> getParsedFlows(String input) {
var res = new LinkedList<ParseResult<Step>>();
String[] splitCRDs = input.split(System.lineSeparator() + "---" + System.lineSeparator());
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this is too weak? Not sure how to make it better, to be honest. This could be in the middle of a long string text. Evil people will do evil things, you know.

Copy link
Member

Choose a reason for hiding this comment

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

yes, haters gonna hate, I know... Would something like this help?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me try :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really working as expected.

@@ -118,7 +118,7 @@ public void populateKamelet(
def.setTitle(String.valueOf(map.getOrDefault("title", "")));
def.setDescription(String.valueOf(map.getOrDefault("description", "")));
def.setRequired((List<String>) map.getOrDefault("required", null));
def.setProperties((Map<String, KameletDefinitionProperty>) map.getOrDefault("required", null));
Copy link
Member Author

Choose a reason for hiding this comment

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

Caught this bug while adding more kamel examples. Oopsie.

@@ -104,7 +105,12 @@ public String parse(final List<Step> steps,
spec.setSteps(null);
}

KameletBinding binding = new KameletBinding(String.valueOf(metadata.getOrDefault("name", "")), spec);
var name = "";
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, we were not very careful and just trusted that things were not null. Naive us.

public String parse(List<StepParserService.ParseResult<Step>> flows) {
StringBuilder sb = new StringBuilder();
flows.stream().forEachOrdered(stepParseResult -> {
if (!sb.isEmpty()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Simple but effective.

@@ -211,6 +212,36 @@ public Node representData(final Object data) {
DumperOptions.FlowStyle.BLOCK);
}
});

this.multiRepresenters.put(KameletDefinitionProperty.class,
new RepresentMap() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was also missing, dumb us.

@@ -55,8 +56,20 @@ public interface DeploymentGeneratorService {
* If applies, the name will be the name used on the integration deployed.
*/
@WithSpan
String parse(List<Step> steps, Map<String, Object> metadata,
List<Parameter> parameters);
@Deprecated
Copy link
Member Author

Choose a reason for hiding this comment

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

deprecated because we should discourage its use.

Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Just to confirm:

1. Response shape

given the following YAML

- from:
    uri: null
    steps:
        - aggregate:
              aggregation-strategy: aggregate

- from:
    uri: null
    steps: []

- from:
    uri: null
    steps: []

we receive an array of routes, which is what I would expect to receive, but just wanted to confirm if this is correct or I should expect an object as you mentioned in another conversation?
image

2. YAML format

Sending 2 integrations in an array, we get the following routes:
image

would be possible / appropriate / useful to give an empty line between the routes? this is extremely 💅 (stylish)

api/pom.xml Show resolved Hide resolved
@Consumes(MediaType.APPLICATION_JSON)
@Produces("text/yaml")
@Path("/")
@CacheResult(cacheName = "api")
Copy link
Member

Choose a reason for hiding this comment

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

How would this cache behave? Is this the correct "doc" for this annotation?

@Override
public List<ParseResult<Step>> getParsedFlows(String input) {
var res = new LinkedList<ParseResult<Step>>();
String[] splitCRDs = input.split(System.lineSeparator() + "---" + System.lineSeparator());
Copy link
Member

Choose a reason for hiding this comment

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

yes, haters gonna hate, I know... Would something like this help?

@lordrip
Copy link
Member

lordrip commented May 5, 2023

BTW, almost forgot, as soon as we get this merged, I'll switch the UI to use the new endpoints and interact only with the first route in the array. After that, I'll start incorporating individual functionality into the multi-route support.

@Delawen
Copy link
Member Author

Delawen commented May 5, 2023

BTW, almost forgot, as soon as we get this merged, I'll switch the UI to use the new endpoints and interact only with the first route in the array. After that, I'll start incorporating individual functionality into the multi-route support.

You like danger 😸 Living on the edge!

@Delawen
Copy link
Member Author

Delawen commented May 5, 2023

Added an object wrapper to return instead of a plain array of flows. You were right.

@sonarcloud
Copy link

sonarcloud bot commented May 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
2.6% 2.6% Duplication

@lordrip
Copy link
Member

lordrip commented May 5, 2023

Alright, now it sends and receives an object like this:

{
  "flows": [ ... ],
  "properties: { ... }
}

@lordrip lordrip self-requested a review May 5, 2023 12:53
@Delawen Delawen enabled auto-merge (rebase) May 5, 2023 14:59
@Delawen
Copy link
Member Author

Delawen commented May 5, 2023

Adding an extra line between routes is breaking tests in an ugly way. I will leave that for a second PR.

@Delawen Delawen merged commit efa5105 into KaotoIO:main May 8, 2023
lordrip added a commit to KaotoIO/kaoto-ui that referenced this pull request May 22, 2023
With the new endpoints introduced by KaotoIO/kaoto-backend#638, we could start wiring up the source code editor and the related components to use the multiple flows approach.

Changes
This pull request replaces the previous v1/integrations endpoint with the new v2/integrations endpoint that supports multiple flows.

All the changes were validated against the existing e2e tests to ensure that the previous functionality from the single flow is still in place.

* chore: Fix settings.js e2e test
* chore: Fix editing.cy.js e2e test
* chore: Fix step_integration.cy.js e2e test
* chore: Fix source_code.cy.js e2e test
* chore: Fix deleting_steps.cy.js e2e test
* chore: Fix catalog_actions.cy.js e2e test
* chore: Fix code_editor_actions.cy.js e2e test
* chore: Fix step_actions_from_canvas.cy.js e2e test
* chore: Fix step_extension_actions_from_canvas.cy.js e2e test
* chore: Fix branches_from_canvas.cy.js e2e test
* chore: Fix branches_step_actions_from_canvas.cy.js e2e test
* chore: Fix nested_branches_from_canvas.cy.js e2e test
* chore: Fix nested_branches_step_actions_from_canvas.cy.js e2e test

relates to: #801
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants