feat: Enable Adaptation Editor for CF projects #3790
Conversation
nikmace
added
preview-middleware
🦋 Changeset detectedLatest commit: a05b3b4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@nikmace Using CF to discover systems and connect is something we already support using axios-extension and https://github.com/SAP/open-ux-tools/blob/main/packages/odata-service-inquirer/src/prompts/datasources/sap-system/abap-on-btp/questions.ts#L216. Is this not sufficent for this use case. Any connectivity features should be added to axios extension, but I believe there is nothing new here? |
Our use case is completely different. We are trying to support Cloud Foundry Adaptation Projects in the Adaptation Editor - that is the goal. We are not having issues with connectivity, and I don't see a reason to enhance axios-extension because we don't need to use any methods from it like we do with AbapServiceProvider (ABAP and Cloud Foundry are two different things). All we need to do is get the service keys from business service, make a call to fetch an authorization token, and attach it to the request headers. Then, authentication is going to happen programatically, service-to-service. For example, for OData, which is linked to a destination and needs authorization. Maybe I don't understand something but I can't see how enhacing axios-extension is going to help us. We are talking about different scenarios, in my opinion. Let's follow up during the meeting. 👍🏻 |
heimwege
left a comment
heimwege
left a comment
There was a problem hiding this comment.
See comments. I just had a look at the backend-proxy-middleware-cf and preview-middleware changes. Can't really comment on the adp packages.
| * @param logger logger instance | ||
| * @throws Error in case no manifest.appdescr_variant found | ||
| */ | ||
| export async function initAdp( |
There was a problem hiding this comment.
Question: Why is initAdp not a function of the class FlpSandbox? As it seems to rely on a specific instance flp: FlpSandbox this could be improved by just making it a function of that instance so that this can be used. Also flp as property name is misleading because there's a config property of the preview-middleware named flp as well.
I know all of this is not related to this PR but something I learned doing the review 🐱
mmilko01
force-pushed
the
feat/3789/cf-adp-support
branch
from
a08292f to
84c8e54
Compare
| await validateConfig(config, logger); | ||
|
|
||
| const tokenProvider = await createTokenProvider(config, logger); | ||
| return setupProxyRoutes(config.paths, config.url, tokenProvider, logger) as unknown as RequestHandler; |
There was a problem hiding this comment.
The cast is not needed because Express's Router type extends RequestHandler
| return setupProxyRoutes(config.paths, config.url, tokenProvider, logger) as unknown as RequestHandler; | |
| return setupProxyRoutes(config.paths, config.url, tokenProvider, logger); |
|
voicis
left a comment
voicis
left a comment
There was a problem hiding this comment.
- Preview middleware changes looks good
- Good code coverage
- Changeset present
- Integration tests passing https://github.com/SAP/open-ux-tools/actions/runs/19763181417
Feat for #3789.