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.

Sign up for GitHub

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

Jump to bottom

[FIX] ui5Framework: Prevent install of libraries within workspace #589

Merged
merged 12 commits into from
Mar 31, 2023
Merged

[FIX] ui5Framework: Prevent install of libraries within workspace #589

merged 12 commits into from
Mar 31, 2023

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Mar 10, 2023 •
edited
Loading

JIRA: CPOUI5FOUNDATION-643

@matz3 matz3 added the bug Something isn't working label Mar 10, 2023
@matz3 matz3 force-pushed the fix-workspace-resolution-framework-libraries branch from 219a640 to 4254fb7 Compare March 10, 2023 14:36
@coveralls
Copy link

coveralls commented Mar 10, 2023 •
edited
Loading

Coverage Status

Coverage: 95.696% (+0.1%) from 95.578% when pulling 563e69a on fix-workspace-resolution-framework-libraries into 7ee6dde on main.

@matz3 matz3 marked this pull request as ready for review March 28, 2023 13:36
@matz3 matz3 requested a review from a team March 28, 2023 13:36
@matz3
Copy link
Member Author

matz3 commented Mar 28, 2023

Ready for review, but tests should be enhanced to cover all the changes.

@d3xter666 d3xter666 requested a review from a team March 29, 2023 07:02
matz3
matz3 commented Mar 29, 2023
View reviewed changes
Copy link
Member Author

@matz3 matz3 left a comment

Choose a reason for hiding this comment

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

I'll create a new PR as prerequisite to get rid of the private API usages

lib/graph/helpers/ui5Framework.js Outdated Show resolved Hide resolved
lib/graph/helpers/ui5Framework.js Outdated Show resolved Hide resolved
@d3xter666 d3xter666 requested a review from a team March 29, 2023 09:54
@d3xter666
Copy link
Contributor

d3xter666 commented Mar 29, 2023 •
edited
Loading

Apart from TODOs & Tests, the code LGTM.

The missing test parts are the new Error in the Abstract resolver and the checks for @sapui5/@openui5 dependencies & optional dependencies

matz3 added a commit that referenced this pull request Mar 29, 2023
@matz3
[FEATURE] Workspace: Add getModules method
46f95b2 
Can be used to retrieve all modules referenced in a workspace to
iterate over them.

Required for #589
matz3 added a commit that referenced this pull request Mar 29, 2023
@matz3
[FEATURE] Specification: Add getId method
e1c83b3 
Returns the ID defined by the provider. Should not be used for general
purposes for which "getName" should still be preferred.

Required for #589
@matz3 matz3 mentioned this pull request Mar 29, 2023
Merged
matz3 added a commit that referenced this pull request Mar 30, 2023
@matz3
[FEATURE] Workspace: Add getModules method
1e2aa0e 
Can be used to retrieve all modules referenced in a workspace to
iterate over them.

Required for #589
matz3 added a commit that referenced this pull request Mar 30, 2023
@matz3
[FEATURE] Specification: Add getId method
7bdb47a 
Returns the ID defined by the provider. Should not be used for general
purposes for which "getName" should still be preferred.

Required for #589
@matz3 matz3 force-pushed the fix-workspace-resolution-framework-libraries branch from 0fdf3cb to 2fb9cb9 Compare March 30, 2023 13:28
@matz3 matz3 requested a review from d3xter666 March 31, 2023 12:32
@matz3 matz3 merged commit 8ffc676 into main Mar 31, 2023
@matz3 matz3 deleted the fix-workspace-resolution-framework-libraries branch March 31, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d3xter666 d3xter666 d3xter666 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matz3 @coveralls @d3xter666