Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[cds^8.2] Event Broker: S/4HANA Cloud to CAP #1198
[cds^8.2] Event Broker: S/4HANA Cloud to CAP #1198
Changes from 17 commits
4dca6ce
a0672d7
e25b0ff
05bcefc
2ce2831
2d21d44
2aae39c
dc16807
ceef211
416ddfd
7cbb407
bc0dcce
84b34c0
8426c9b
838915e
6fe2b8b
7f63365
c910b64
a37e6a4
84ddc70
224d651
25bd054
0f4e543
8bc984a
ff06536
25ad6fe
6c3b729
0d28f43
3aaa254
f3fa527
003a7c2
71dbfde
e3aa0a6
3fe7fc9
5223357
0d11bcc
7cd089d
4978cf7
5980ac4
ed919e2
84214ca
480e645
d71a516
4d8d872
61778ad
09c8b52
9f2eb23
7c5bebd
4fc364f
2f59393
e19c1a4
56b5e50
928d43c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@renejeglinsky @smahati we need to replace with a link to a published doc before releasing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not true, it must be in an external service. Otherwise it's considered to be an event the app publishes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention IAS requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wouldn't in the stack agnostic messaging guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... but it's specific to event broker already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just refer to the event broker section in messaging, they should look it up there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention the 'trust establishment' issues (race conditions), "might take some time, try again later"... bad to write, but not mentioning it would lead to lots of frustration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho, not a cap topic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alper and I tried it again and the race conditions didn't appear again... hence I would just not mention it atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That whole chapter should preferably move to the Event Broker team's deployment guide, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their current guide is incomprehensible and doesn't provide enough information for anyone to deploy an app with IAS-based authentication, so it's good that CAP provides a sample
mta.yaml
file which connects all dots. There are also some CAP-specific configurations, like thewebhookUrl
property.In the future, I hope we will have proper
cds add event-broker
andcds add identity
support to cover it. But I think we should move the samplemta.yaml
file to our event broker section. We did a similar thing for Event Mesh (at least the XSUAA configuration).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, I dislike having S/4HANA specifics in our guide as well as instructions on how to configure systems and formations in the BTP Cockpit System Landscape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I therefore propose:
Move a link to a sample mta.yaml to the event-broker section (not this guide here), explain the necessary services (e.g. IAS) and the reasoning behind it. This guide should only reference the event broker section for specifics of its configuration & deployment.
Clarify with the Event Broker colleagues that all information regarding the configuration of the system landscape and enablement of the events is properly documented in their docs. Then we can add a link to it.
Same goes for instructions on how to configure S/4HANA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test event is quite prominent, shouldn't we mention that the actual to-be-consumed event types need to be declared here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, readers might be confused that it works for the test event, but not for the actual event they later might want to consume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be part of event broker docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. Create Formation
missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, formation will already exist (event broker docs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formation needs to be manually created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only with
ngrok
etc. But since webhook urls are specified as binding parameters, it's cumbersome to quickly switch them for testing. I therefore wouldn't mention it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, same thinking here. but i wanted to keep as a kind of reminder that this could be improved...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's also required to have an IAS service, I think we should mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also just link to the
event-broker
section, there it's written thatIAS
is needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
→ that would use our default env preset