-
Notifications
You must be signed in to change notification settings - Fork 23
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] flexChangesBundler: Add flexibility-bundle.json #353
Conversation
// if there are ctrl variant and minUI5Version < 1.72 need to update minUI5Version to 1.72.0 | ||
if (hasVariant && !hasFlexBundleVersion) { | ||
log.verbose("minUI5Version < 1.72, updated it to 1.72.0"); | ||
data["sap.ui5"].dependencies.minUI5Version = "1.72.0"; |
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, the tooling must not change manifest metadata on the fly, it should rather stop with an error.
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.
Thanks, I change the logic.
The build will stop when the minUI5Version is below 1.72 and there are control variants in the change folder
Please prefix your commit message with |
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.
Basically okay with me. Most comments are style comments (a matter of taste). The most important point IMO is the missing explanation of the new format. What files are stored in what array etc.
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.
LGTM now.
Had a rough look, lgtm. @codeworrior feel free to merge 👍 |
Just wondering, is this an [INTERNAL] or a [FEATURE] change? |
Released as part of UI5 CLI |
Hi All, What should be done to avoid this error? |
@shahzeb79 I would say, the error message is quite instructive, isn't it? Or aren't there any variant changes in the app or does it already use a min version of 1.73.0 or higher? Can you please point us to the app's sources or to a build log, for further analysis? |
Hi, Here is the link for the app having this error. <removed internal URL from public GitHub - RandomByte> |
I see. I guess the Maven property Is Maven filtering (or an equivalent) active in your build before the ui5 tooling is executed? As the project resources rely on Maven properties, such a step is required anywhere. The ui5 tooling to my knowledge only can replace the |
No. Of course you could add replacement for other strings in a custom middleware however. |
@codeworrior no we dont do any mvn thing before ui5tooling now. Do you think we have to issue some mvn command ? |
But as there's no way to write back the modified resources it's not possible for the flexChangesBundler to receive it.
You might process the resources via maven (with filtering) and then use the "target" folder for running the UI5 Tooling and karma tests |
I think I meant custom task. There this should be possible, shouldn't it? |
Yes, there it's possible. |
@matz3 Yes we can run mvn to generate target |
@shahzeb79 have you seen my comment in your other issue: SAP/ui5-tooling#245 (comment) ? I think what @matz3 suggests might be a good workaround but the better solution would be the outcome of SAP/ui5-tooling#245 (comment) where you can work with flex changes directly from source. |
[INTERNAL] If minUI5Version >= 1.73 flexibility-bundle.json is create indeed of changes-bundle.json
If minUI5Version < 1.73 and there are no control variant changes changes-bundle.json is create.
If minUI5Version < 1.73 and there are control variant changes the build will throw an error.