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] Add Configuration #575

Merged
merged 18 commits into from
Apr 19, 2023
Merged

[FEATURE] Add Configuration #575

merged 18 commits into from
Apr 19, 2023

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented Feb 13, 2023

Introduce a new Configuration module for persisting UI5 Project specific settings.
JIRA: CPOUI5FOUNDATION-634

Complements: #570

@coveralls
Copy link

coveralls commented Mar 6, 2023

Coverage Status

Coverage: 94.991% (-0.02%) from 95.008% when pulling f37a62d on add-configuration into 6a09bc9 on main.

* @param {string} [filePath="~/.ui5rc"] Path to configuration JSON file
* @param {@ui5/project/config/Configuration} config Configuration to save
*/
export async function saveConfig(filePath, config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's clear from the RFC that this should be part of the CLI.

Would be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the latest discussions and the future integration with Snapshot Consumption, it's worth it that the Config object would be able to read and write into the .ui5rc file.

The core reasoning behind that is the following scenario:

  1. There's an exported static function that reads the file, creates and returns the Configuration object.

  2. Writing in .ui5rc is a bit more complex: if no .ui5rc config is provided, then the Resolver checks for env variable, but there's a last resort if none of these is provided (new project setup, for example). In such a case, it's worth checking for settings.xml in case the end-user has maven installed on their machine. Then we need to read the settings.xml, extract the URL and save it to .ui5rc

We have decided to give it a try. Furthermore, the Configuration object is closed for now and we could eventually revise it

}


export async function resolveSnapshotEndpointUrl(settingsXML, skipConfirmation) {
Copy link
Contributor

@d3xter666 d3xter666 Mar 10, 2023

Choose a reason for hiding this comment

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

We've discussed this and need to look at it from the perspective of the developers.
We do not expect them to have Maven installed.

With the potential CLI implementation of the configuration settings, it might become redundant.
Developers would eventually need just to execute once a single CLI command like ui5 config set snapshotUrl https://..... when setting up their environment for the first time.

If we decide that we still need this revolver, its place must be somewhere in the mvn/Installer where it actually is in use.
This would raise another question, though- how to store the resolved value?

Comment on lines 67 to 83
this.#cwd = cwd ? path.resolve(cwd) : process.cwd();
this.#sources = !!sources;
this.#version = version;
this.#cacheMode = cacheMode;
this.#ui5HomeDir = ui5HomeDir ? path.resolve(ui5HomeDir) : path.join(os.homedir(), ".ui5");

this.#snapshotEndpointUrl = process.env.UI5_MAVEN_SNAPSHOT_ENDPOINT || snapshotEndpointUrl;
this.#frameworkDir = frameworkDir ? path.resolve(frameworkDir) : path.join(this.#ui5HomeDir, "framework");
this.#artifactsDir = artifactsDir ?
path.resolve(artifactsDir) : path.join(this.#frameworkDir, "artifacts");
this.#packagesDir = packagesDir ?
path.resolve(packagesDir) : path.join(this.#frameworkDir, "packages");
this.#metadataDir = metadataDir ?
path.resolve(metadataDir) : path.join(this.#frameworkDir, "metadata");
this.#stagingDir = stagingDir ? path.resolve(stagingDir) : path.join(this.#frameworkDir, "staging");
this.#lockDir = lockDir ? path.resolve(lockDir) : path.join(this.#frameworkDir, "locks");
this.#cacheDir = cacheDir ? path.resolve(cacheDir) : path.join(this.#frameworkDir, "cacache");
Copy link
Contributor

@d3xter666 d3xter666 Mar 10, 2023

Choose a reason for hiding this comment

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

There are several questions that arise here.

Which properties/settings should be within the Configuration object?

Currently, all the needed configuration options are handled here. The reasoning is that many depend on each other i.e. frameworkDir depends on ui5HomeDir

On the other side, the core reasoning to introduce such a Configuration was to support the Snapshot URL.

Do we need default values?

According to the spec, there shouldn't be default values.

Properties could be defined/provided separately. But the way we use them internally implies that they depend on each other i.e. frameworkDir depends on ui5HomeDir.
Therefore, there's no ambiguity for those values and they could be easily derived.

Also, after looking into the code, it could be stated that the ui5HomeDir should be ~/.ui5

Copy link
Contributor

Choose a reason for hiding this comment

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

For this iteration, we have decided to go only with the necessary snapshotEndpointUrl. The questions above would be resolved in separate iteration(s)/BLI(s).

d3xter666 added a commit that referenced this pull request Mar 13, 2023
@d3xter666 d3xter666 changed the title [INTERNAL][POC] Add Configuration.js [INTERNAL] Add Configuration.js Mar 28, 2023
@d3xter666 d3xter666 requested review from a team and d3xter666 and removed request for d3xter666 April 2, 2023 09:03
lib/config/Configuration.js Outdated Show resolved Hide resolved
lib/config/Configuration.js Outdated Show resolved Hide resolved
d3xter666 added a commit that referenced this pull request Apr 3, 2023
@d3xter666 d3xter666 marked this pull request as ready for review April 4, 2023 11:34
@d3xter666 d3xter666 requested a review from a team April 4, 2023 11:35
@d3xter666 d3xter666 requested a review from matz3 April 5, 2023 10:44
lib/config/Configuration.js Outdated Show resolved Hide resolved
lib/config/Configuration.js Outdated Show resolved Hide resolved
lib/config/Configuration.js Outdated Show resolved Hide resolved
lib/config/Configuration.js Outdated Show resolved Hide resolved
matz3
matz3 previously requested changes Apr 5, 2023
lib/config/Configuration.js Outdated Show resolved Hide resolved
lib/config/Configuration.js Outdated Show resolved Hide resolved
@d3xter666 d3xter666 changed the title [INTERNAL] Add Configuration.js [FEATURE] Add Configuration.js Apr 6, 2023
Copy link
Member Author

@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, just a small remark

test/lib/config/Configuration.js Outdated Show resolved Hide resolved
d3xter666 added a commit that referenced this pull request Apr 12, 2023
d3xter666 added a commit that referenced this pull request Apr 12, 2023
@RandomByte RandomByte force-pushed the add-configuration branch 2 times, most recently from a6508b6 to b1bc70f Compare April 17, 2023 14:03
@RandomByte
Copy link
Member Author

Rebased to resolve conflict with main

@RandomByte RandomByte merged commit fd37cef into main Apr 19, 2023
@RandomByte RandomByte deleted the add-configuration branch April 19, 2023 08:30
d3xter666 added a commit to SAP/ui5-cli that referenced this pull request Apr 19, 2023
RandomByte pushed a commit to SAP/ui5-cli that referenced this pull request Apr 21, 2023
RandomByte pushed a commit to SAP/ui5-cli that referenced this pull request Apr 21, 2023
d3xter666 added a commit to SAP/ui5-cli that referenced this pull request Apr 24, 2023
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.

4 participants