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] Enable snapshot consumption from Maven repository #570

Merged
merged 128 commits into from
Apr 17, 2023
Merged

Conversation

d3xter666
Copy link
Contributor

@d3xter666 d3xter666 commented Feb 7, 2023

This is just a rebase with main of Enable snapshot consumption from Maven repository #569. The original change had conflicts with the workspace implementation.

Implementation of snapshot consumption from Maven repository. This is the final implementation and is the continuation work from the base change: #478

The change also implements the RFC for a Configuration
The configuration part has been moved into a separate change: #575

JIRA: CPOUI5FOUNDATION-581

@d3xter666 d3xter666 changed the title Rebase of #478 [INTERNAL] Rebase of #478 (Enable snapshot consumption from Maven repository) Feb 8, 2023
@d3xter666 d3xter666 requested review from RandomByte and a team February 8, 2023 12:57
@coveralls
Copy link

coveralls commented Feb 9, 2023

Coverage Status

Coverage: 95.008% (-0.7%) from 95.696% when pulling 1e22e0e on rebase-2 into f68df28 on main.

lib/config/Configuration.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/maven/Installer.js Outdated Show resolved Hide resolved
lib/ui5Framework/maven/Installer.js Show resolved Hide resolved
lib/ui5Framework/maven/Registry.js Outdated Show resolved Hide resolved
lib/ui5Framework/npm/Installer.js Outdated Show resolved Hide resolved
lib/ui5Framework/npm/Registry.js Outdated Show resolved Hide resolved
lib/config/Configuration.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/graph/helpers/ui5Framework.js Outdated Show resolved Hide resolved
lib/ui5Framework/npm/Registry.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/maven/Registry.js Outdated Show resolved Hide resolved
lib/ui5Framework/maven/Registry.js Outdated Show resolved Hide resolved
@d3xter666 d3xter666 requested a review from matz3 March 31, 2023 08:39
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/maven/Installer.js Show resolved Hide resolved
lib/ui5Framework/Openui5Resolver.js Outdated Show resolved Hide resolved
test/lib/graph/helpers/ui5Framework.js Outdated Show resolved Hide resolved
test/lib/ui5framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
@d3xter666 d3xter666 requested a review from matz3 April 3, 2023 07:15
lib/graph/helpers/ui5Framework.js Outdated Show resolved Hide resolved
lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/AbstractResolver.js Outdated Show resolved Hide resolved
lib/ui5Framework/maven/Installer.js Outdated Show resolved Hide resolved
test/lib/graph/helpers/ui5Framework.js Outdated Show resolved Hide resolved
lib/ui5Framework/maven/Installer.js Outdated Show resolved Hide resolved
@d3xter666 d3xter666 requested a review from matz3 April 5, 2023 10:24
Copy link
Member

@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.

LGTM. Title / commit message still needs to be adopted + UA review

@d3xter666 d3xter666 requested a review from KlattG April 5, 2023 10:40
@d3xter666 d3xter666 changed the title [INTERNAL] Rebase of #478 (Enable snapshot consumption from Maven repository) [INTERNAL] Enable snapshot consumption from Maven repository Apr 5, 2023
@d3xter666 d3xter666 changed the title [INTERNAL] Enable snapshot consumption from Maven repository [FEATURE] Enable snapshot consumption from Maven repository Apr 6, 2023
@RandomByte
Copy link
Member

Hi @KlattG, I updated some user-facing texts and would appreciate your feedback. Here's the delta: 8e3f4c4 (#570)

RandomByte
RandomByte previously approved these changes Apr 13, 2023
Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

LGTM

KlattG
KlattG previously approved these changes Apr 13, 2023
Copy link
Contributor

@KlattG KlattG left a comment

Choose a reason for hiding this comment

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

LGTM

ui5HomeDir: path.resolve(
ui5HomeDir || path.join(os.homedir(), ".ui5")
),
snapshotEndpointUrlCb: this._snapshotEndpointUrlCb,
Copy link
Member

Choose a reason for hiding this comment

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

Since this method is static, this._snapshotEndpointUrlCb is undefined and needs to be provided as a parameter.

I noticed this only in a manual test locally. I'll add a commit fixing this

@RandomByte RandomByte requested a review from matz3 April 14, 2023 09:16
@@ -208,6 +208,13 @@ class Sapui5MavenSnapshotResolver extends AbstractResolver {
}
}

if (!process.stdout.isTTY) {
// Do not prompt if stdout is non-interactive (i.e. in CI environments) and just use the URL
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this against the idea to first ask for confirmation to use the URL?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was asking myself that too. Since we can't ask the user in this case, we need to decide upfront.

I went with using the Maven config since I expect this to make things easier in CI environments where the config is already present (I know of some base images where this is the case). But you're right, it might be unwanted or unexpected.

However, the only alternative I see is to always ignore the Maven configuration in CI environments, forcing users to configure the environment variable or ui5 config.

lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
…ame function, do not resolve if sdtout is non-interactive
…s module

This caused the creation of an empty directory structure for the tests
@RandomByte RandomByte dismissed stale reviews from flovogt and themself April 17, 2023 12:34

Obsolete

@RandomByte RandomByte merged commit ade2c49 into main Apr 17, 2023
@RandomByte RandomByte deleted the rebase-2 branch April 17, 2023 12:35
RandomByte pushed a commit that referenced this pull request Apr 17, 2023
RandomByte pushed a commit that referenced this pull request Apr 18, 2023
RandomByte added a commit that referenced this pull request Apr 19, 2023
Introduce a new Configuration module for persisting UI5 Project specific settings.

Complements: #570
JIRA: CPOUI5FOUNDATION-634
Co-authored-by: Yavor Ivanov <yavor.ivanov@sap.com>
RandomByte added a commit to SAP/ui5-cli that referenced this pull request Apr 25, 2023
This feature depends on SAP/ui5-project#575 and makes the configuration of the `mavenSnapshotEndpointUrl` option possible, which is used by  SAP/ui5-project#570

Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants