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

feature(#958): default kafka topics for all integrations #1168

Merged
merged 31 commits into from
Jan 29, 2024

Conversation

lmoesle
Copy link
Contributor

@lmoesle lmoesle commented Jan 2, 2024

Description

  • Introduce app_integration_name header
  • Configurable default destination for integrations
  • Remove app_message_name header

Reference

Issues:

Check-List

  • All Acceptance criteria of user story are met
  • JUnit tests are written (60% CodeCov)
  • Internal Review is maintained
  • Documentations external and internal are completed
  • Smoketest successful (Manual E2E-Test depending on Change)
  • No Branch waste left
  • Board is up-to-date
  • Openshift environments are prepared (Secrets, etc.) and release-issue is maintained

Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (b8a1ad5) 38.97% compared to head (b76d089) 39.38%.

❗ Current head b76d089 differs from pull request most recent head 4ed9284. Consider uploading reports for the commit 4ed9284 to get more accurate results

Files Patch % Lines
.../process/AbstractStreamingIntegrationDelegate.java 0.00% 8 Missing ⚠️
...nnector/adapter/camunda/rest/in/CamundaClient.java 63.15% 6 Missing and 1 partial ⚠️
.../rest/out/processdefinition/ServiceInstanceTO.java 30.00% 7 Missing ⚠️
...giwf/connector/core/DigiWFConnectorProperties.java 0.00% 3 Missing ⚠️
...adapter/out/integration/IntegrationOutAdapter.java 0.00% 3 Missing ⚠️
...adapter/out/integration/IntegrationOutAdapter.java 0.00% 3 Missing ⚠️
...or/core/adapter/in/streaming/IncidentConsumer.java 50.00% 1 Missing and 1 partial ⚠️
...stance/api/resource/ServiceInstanceController.java 0.00% 2 Missing ⚠️
...ctor/adapter/camunda/rest/out/IncidentAdapter.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #1168      +/-   ##
============================================
+ Coverage     38.97%   39.38%   +0.40%     
- Complexity     1315     1344      +29     
============================================
  Files           595      600       +5     
  Lines          8667     8732      +65     
  Branches        427      430       +3     
============================================
+ Hits           3378     3439      +61     
- Misses         5152     5155       +3     
- Partials        137      138       +1     
Flag Coverage Δ
unittests 39.38% <72.93%> (+0.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lmoesle lmoesle force-pushed the feature/default-kafka-topics-for-all-integrations-#958 branch 3 times, most recently from b467d12 to 70456d3 Compare January 12, 2024 07:54
@lmoesle lmoesle force-pushed the feature/default-kafka-topics-for-all-integrations-#958 branch from 70456d3 to 4bb2f59 Compare January 15, 2024 16:37
@lmoesle lmoesle force-pushed the feature/default-kafka-topics-for-all-integrations-#958 branch from 171a941 to b7b044e Compare January 23, 2024 07:59
@lmoesle
Copy link
Contributor Author

lmoesle commented Jan 24, 2024

@lmoesle
Copy link
Contributor Author

lmoesle commented Jan 24, 2024

Integrationen

  • Address Integration
  • ALW Integration
  • Cosys Integration
  • DMS Integration
  • Email Integration
  • S3 Integration

Prozesse

  • Example Email Integration
  • example-email-V02
  • Example Address Integration
  • Example Cosys Generate Document -> Fehler siehe unten, hat aber nichts mit dem Feature zu tun
  • Example Cosys Generate Document (Streaming) -> Fehler siehe unten, hat aber nichts mit dem Feature zu tun
  • Feature S3 Integration -> geht bis auf Cosys

Ich kann Cosys lokal nicht starten ->realm does not exist Fehler (hat aber nichts mit dem Change zu tun

Bei ALW habe ich keine Ahnung wie man das testet

@StephanStrehlerCGI
Copy link
Contributor

Funktioniert bei mir mit Mail Integration.
Passt schon

@lmoesle lmoesle force-pushed the feature/default-kafka-topics-for-all-integrations-#958 branch from 40ed80a to b76d089 Compare January 29, 2024 14:15
@lmoesle lmoesle enabled auto-merge (squash) January 29, 2024 14:24
@lmoesle lmoesle disabled auto-merge January 29, 2024 14:25
@lmoesle lmoesle enabled auto-merge (squash) January 29, 2024 14:51
@lmoesle lmoesle merged commit 406a484 into dev Jan 29, 2024
5 checks passed
@lmoesle lmoesle deleted the feature/default-kafka-topics-for-all-integrations-#958 branch January 29, 2024 15:18
@Override
public String loadProcessDefinition(final String processInstanceId) throws ProcessDefinitionLoadingException {
try {
return processInstanceClient.getRootProcessInstanceDetail(processInstanceId).getDefinitionName();
Copy link
Member

Choose a reason for hiding this comment

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

@lmoesle was mir gerade aufgefallen ist, warum nutzt du den hier den Name und nicht den Key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ich habe mich an der getProcessInstanceDetail Methode orientiert und das gleiche auch für die Root Instance hinzugefügt (https://github.com/it-at-m/digiwf-core/blob/dev/digiwf-engine/digiwf-engine-service/src/main/java/de/muenchen/oss/digiwf/process/instance/api/resource/ServiceInstanceController.java). Ich habe dann die ServiceInstanceTO Klasse wiederverwendet und die hatte keinen DefinitionKey, sondern nur den DefinitionName.

Mir ist aber beim Testen kein Unterschied zwischen dem DefinitionName und DefinitionKey aufgefallen, deswegen habe ich das so gemacht.

Copy link
Member

@simonhir simonhir Feb 15, 2024

Choose a reason for hiding this comment

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

In den realen Prozessen sind die Keys immer eindeutiger oft mit Prefix für alle zusammen gehörenden Prozesse und die Namen mit Leerzeichen und deutlich umfangreicher. Müsste man auf jeden Fall spätestens im Zuge der wirklichen Policies einbauen, aber ist für den Moment wahrscheinlich noch nicht so relevant.

Soweit ich das gesehen habe sollte das ja auch super einfach einbauen zu sein, weil die Engine den eigentlich mitgibt und nur die TO-Klasse den Key verwirft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ich ändere das ab 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants