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 config command #618

Merged
merged 15 commits into from
Apr 25, 2023
Merged

[FEATURE] Add config command #618

merged 15 commits into from
Apr 25, 2023

Conversation

d3xter666
Copy link
Contributor

@d3xter666 d3xter666 commented Apr 4, 2023

This feature depends on SAP/ui5-project#575
and makes the enablement of mavenSnapshotEndpointUrl easier: SAP/ui5-project#570

@d3xter666 d3xter666 requested a review from a team April 4, 2023 12:45
@coveralls
Copy link

coveralls commented Apr 5, 2023

Coverage Status

Coverage: 96.704% (+0.06%) from 96.641% when pulling 04fdf8d on add-configuration-cli into a5fe4e4 on main.

@d3xter666 d3xter666 changed the title [FEATURE] Add confg command [INTERNAL] Add confg command Apr 5, 2023
@d3xter666 d3xter666 changed the title [INTERNAL] Add confg command [INTERNAL] Add config command Apr 5, 2023
@d3xter666 d3xter666 changed the title [INTERNAL] Add config command [FEATURE] Add config command Apr 6, 2023
@d3xter666
Copy link
Contributor Author

Note: The build would fail until we merge and make a release of: SAP/ui5-project#575

lib/cli/commands/config.js Outdated Show resolved Hide resolved
lib/cli/commands/config.js Outdated Show resolved Hide resolved
flovogt
flovogt previously approved these changes Apr 21, 2023
matz3
matz3 previously approved these changes Apr 21, 2023
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.

Otherwise LGTM

lib/cli/commands/config.js Outdated Show resolved Hide resolved
@RandomByte RandomByte dismissed stale reviews from matz3 and flovogt via 21949aa April 21, 2023 15:44
@RandomByte RandomByte force-pushed the add-configuration-cli branch 2 times, most recently from 21949aa to 92e29ba Compare April 21, 2023 16:14
* No help is printed on yargs errors, so do not refer to it
  (also see #256)
* Only mention 'ui5 --help'
* Fix 'ui5 set <key>' without value or empty string to unset
* Log everything to stderr, except machine parsable output like
  configuration values returned by 'ui5 list' and 'ui5 get <key>'
* Do not mention location of ui5rc in config commands. In the future we
  might have multiple locations and @ui5/project/config/Configuration
  would take care of deciding where to read what from. The CLI should
  not have that knowledge
* Move imports and variable definitions to handler in order to only
  execute them if the command is actually 'ui5 config'
* Tests now mock Configuration in order to not modify the actual
  configuration of the user running the tests
lib/cli/commands/config.js Outdated Show resolved Hide resolved
const command = commandArgs[commandArgs.length - 1];

const {default: Configuration} = await import( "@ui5/project/config/Configuration");
const allowedKeys = ["mavenSnapshotEndpointUrl"];
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this information come from the Configuration class instead of hard-code it here?
It would allow us to add new options without having to adjust the CLI project.

Copy link
Member

Choose a reason for hiding this comment

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

True. That's a good idea we should follow-up on 👍

@RandomByte RandomByte requested a review from KlattG April 24, 2023 13:05
lib/cli/commands/config.js Outdated Show resolved Hide resolved
test/lib/cli/commands/config.js Outdated Show resolved Hide resolved
test/lib/cli/commands/config.js Outdated Show resolved Hide resolved
test/lib/cli/commands/config.js Outdated Show resolved Hide resolved
test/lib/cli/commands/config.js Outdated Show resolved Hide resolved
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
@RandomByte RandomByte merged commit 9910e30 into main Apr 25, 2023
@RandomByte RandomByte deleted the add-configuration-cli branch April 25, 2023 13:53
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