Conversation
…and remove streamx publish/unpublish/cloud/dev/init commands for now)
| uses: actions/setup-java@v3 | ||
| with: | ||
| java-version: '17' | ||
| java-version: '21' |
There was a problem hiding this comment.
The builds fail on java 17 when importing com.streamx ingestion client that's built with java 21
| @Override | ||
| public Map<String, String> createSubstitutionVariables(String payloadPath, | ||
| String channel, String relativePath) { | ||
| String eventType, String eventSource, String relativePath) { |
There was a problem hiding this comment.
In the eventsource.yaml files:
channelis now replaced witheventType- and
eventSourceis added to them, as a mandatory field in CloudEvents API
| private final Map<String, Publisher<JsonNode>> publishersCache = new HashMap<>(); | ||
| private final Map<String, String> schemaTypesCache = new HashMap<>(); | ||
| // TODO "_v2" is a temporary postfix for now | ||
| public static final String COMMAND_NAME = "batch_v2"; |
There was a problem hiding this comment.
Added those postfixes instead of adding an additional experimental word in the command, to not break the current limitations that each command has a single word name
| @Inject | ||
| IngestionMessageJsonFactory ingestionMessageJsonFactory; | ||
|
|
||
| private State state; |
There was a problem hiding this comment.
Now a method parameter instead of a mutable field
| if (publisher == null) { | ||
| publisher = client.newPublisher(getChannel(), JsonNode.class); | ||
| publishersCache.put(getChannel(), publisher); | ||
| } |
There was a problem hiding this comment.
Now a single publisher
| @ConfigProperty(name = "streamx.cli.e2e.web.delivery.url", defaultValue = "http://localhost:8087/") | ||
| String webDeliveryPortUrl; | ||
| @ConfigProperty(name = "streamx.cli.e2e.nginx.url", defaultValue = "http://localhost:8089/overridden/") | ||
| String nginxPortUrl; |
There was a problem hiding this comment.
To make things simplier maybe we don't need to test both web sink and nginx in CLI - this is rather the responsibility of streamx-service-mesh/runner (and its tested there)
| CLI_SHORT_TIMEOUT_IN_SEC) | ||
| ); | ||
| String url = webDeliveryPortUrl + resourcePath; | ||
| httpValidator.validate(url, expectedStatusCode, expectedBody, CLI_TIMEOUT); |
There was a problem hiding this comment.
Testing just in the Web Sink is enough 💭
| @@ -0,0 +1 @@ | |||
| <h1>Hello World!</h1> | |||
There was a problem hiding this comment.
...could have empty content, since it's for unpublishing - but still the file must exist to be picked up
pom.xml
Outdated
| <streamx.version>1.0.16</streamx.version> | ||
| <ingestion-client.version>1.1.1</ingestion-client.version> | ||
| <streamx-operator.version>0.0.16</streamx-operator.version> | ||
| <streamx.version>2.0.6</streamx.version> |
There was a problem hiding this comment.
Merge client and runner versions to single release of streamx-service-mesh
pom.xml
Outdated
| <executions> | ||
| <execution> | ||
| <goals> | ||
| <goal>integration-test</goal> |
There was a problem hiding this comment.
Enable running the e2e tests
| import dev.streamx.cli.command.ingestion.batch.EventSourceDescriptor; | ||
| import dev.streamx.cli.command.ingestion.batch.exception.EventSourceDescriptorException; | ||
| import dev.streamx.cli.util.FileUtils; | ||
| import io.quarkus.logging.Log; |
There was a problem hiding this comment.
This class makes jacoco coverage measurer lost - makes the whole class have 0% coverage, which may make it look unused
This also fixes the
[WARNING] Rule violated for bundle streamx-cli-core: instructions covered ratio is 0.65, but expected minimum is 0.70
|
|
||
| public class EventSourceFileTreeWalker extends SimpleFileVisitor<Path> { | ||
|
|
||
| private final Logger logger = Logger.getLogger(EventSourceFileTreeWalker.class); |
There was a problem hiding this comment.
Other classes in this project also do this
2942184 to
16501c8
Compare
|
|
||
| @Inject | ||
| MeshDefinitionResolver uut; | ||
|
|
There was a problem hiding this comment.
Added test for interpolating with default value, such as:
ref: "${config.source.interpolated:inbox.pages}"It works as expected 👍
| System.setProperty("config.image.interpolated", "value"); | ||
| void shouldResolveWithMandatoryAndOptionalPropertiesDefined() throws IOException { | ||
| System.setProperty("config.image.interpolated", "image-1"); | ||
| System.setProperty("config.source.interpolated", "source-1"); |
There was a problem hiding this comment.
Default value is overridden here
.github/workflows/ci-test-build.yaml
Outdated
| - name: Build project | ||
| run: | | ||
| ./mvnw clean verify -P all-tests | ||
| ./mvnw clean verify -P all-tests -Dnative |
There was a problem hiding this comment.
leverage the multi-arch feature to allow running mesh (in integration tests) also on both linux and mac host systems
|
|
||
| private static final Set<String> COMMANDS_REQUIRING_PRINTING_BANNER = | ||
| Set.of(DevCommand.COMMAND_NAME, RunCommand.COMMAND_NAME); | ||
| Set.of(RunCommand.COMMAND_NAME); |
There was a problem hiding this comment.
The not migrated (so - removed) commands are
- dev
- cloud (deploy/undeploy)
- init
- publish
- unpublish
| doRun(client); | ||
| } catch (UnsupportedChannelException e) { | ||
| throw new ParameterException(spec.commandLine(), e.getMessage()); | ||
| } catch (StreamxClientConnectionException e) { |
There was a problem hiding this comment.
Those exception types are gone
| } catch (StreamxClientConnectionException e) { | ||
| throw new UnableToConnectIngestionServiceException(ingestionClientConfig.url(), e); | ||
| Publisher publisher = client.newPublisher(); | ||
| perform(publisher); |
There was a problem hiding this comment.
Inlined the doRun method here
| state.eventSubject, | ||
| state.eventType, | ||
| state.eventSource, | ||
| state.payload // TODO rename payload to data everywhere |
There was a problem hiding this comment.
I think worth doing this in a separate PR, since a lot of files would get changed, making the PR too hard to read. Internally "payload" is assigned as cloud event's "data" ✅
| .withDataContentType("application/json") | ||
| .withData(JsonCloudEventData.wrap(data)) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
Needed only by the batch command to create the event from event descriptor and data payload files
| <directory>src/test/resources/filtered</directory> | ||
| <includes> | ||
| <include>mesh.yaml</include> | ||
| <include>overridden-nginx-conf</include> |
There was a problem hiding this comment.
Maybe no need to test this feature in the CLI, it's tested at source projects
| String responseBody = EntityUtils.toString(httpEntity); | ||
| assertThat(responseBody).describedAs(url).contains(expectedBody); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Added an overload to test also binary expected body (to verify an image will be published to StreamX with unchanged content)
| String responseBody = EntityUtils.toString(response.getEntity()); | ||
| logger.info("Request to " + url | ||
| + " return statusCode " + actualStatusCode | ||
| + " and body " + responseBody); |
There was a problem hiding this comment.
Thanks to assertj .describedAs() as well as its descriptive error messages - we don't need to log manually anymore
| .describedAs(() -> "Full output is:\n" | ||
| + String.join("\n", process.getCurrentOutputLines()) | ||
| + "\n" | ||
| + String.join("\n", process.getCurrentErrorLines())) |
There was a problem hiding this comment.
Adding very helpful logs.
Until now, the tests tested that "java -jar"'s process success output logs contain expected text.
Now - when they don't - the full process output (including errors) is logged
| } | ||
|
|
||
| private void runStreamxCommand(String command, String expectedOutput, long timeoutInS) { | ||
| private void runStreamxCommand(String command, String expectedOutput, Duration timeout) { |
There was a problem hiding this comment.
No need to specify the timeout unit in parameter name, if we can use Duration object
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("testCases") |
There was a problem hiding this comment.
Those were test cases for publish command, and it was decided that
- we remove
publishandunpublish - because no one uses them
- and they handle just a single ingestion while the jar start time is long
- we don't want to manage the wide variety of options described in this method
Instead, the test class was modified to verify end-to-end:
- batch publish and unpublish
- stream publish and unpublish
| ); | ||
|
|
||
| validateStreamxPage("ds.png", | ||
| Files.readAllBytes(Path.of("src/test/resources/batch/publish/asset/ds.png"))); |
There was a problem hiding this comment.
The image is published to real StreamX and here we verify that when the image is downloaded via http from web sink service url - the bytes are unchanged
| .map(File::toPath) | ||
| .map(Path::toAbsolutePath) | ||
| .map(Path::normalize) | ||
| .map(Path::toString) |
There was a problem hiding this comment.
Remove any "/../" from the logged path
| processes.add(shellProcess); | ||
| return shellProcess; | ||
| } catch (IOException e) { | ||
| logger.info("Error running terminal command: " + command); |
There was a problem hiding this comment.
Deduplicate log messages for success
49665df to
0158222
Compare
47d8f03 to
a0b9cc6
Compare
📋 Type of the Changes
🛠 Changes being made
Added streamx commands for StreamX 2.0 (and removed the original
run/stream/batch):run_v2stream_v2batch_v2Removed streamx commands:
publishandunpublish: permanently, as discussedcloud,devandinit: removed for now, since adjusting them to StreamX 2.0 was not part of the task, but need the whole codebase to compile and pass tests✅ Checklist