-
Notifications
You must be signed in to change notification settings - Fork 35
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
2128/confirm prompt validator not invoked as expected #2378
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 3df07ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
be5a3dd
to
0fdd6b4
Compare
b64bd12
to
0e0aa15
Compare
7ddbb0a
to
b244201
Compare
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.
Update looks good
Not manually tested
Tests to cover
Changeset ✅
packages/abap-deploy-config-inquirer/src/prompts/questions/abap-target.ts
Outdated
Show resolved
Hide resolved
df87e09
to
461741d
Compare
1f18bc2
to
55782f8
Compare
Quality Gate passedIssues Measures |
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.
Re-approving
} | ||
|
||
/** | ||
* Determines if the client question should be shown. | ||
* Note: In some instances, when a yaml conf is parsed, double quoted properties i.e. client: "100" are saved as a number instead of a string. | ||
* Determines if the client question should be shown under very specific conditions. |
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.
Which are the conditions in plain terms as the code is hard to follow here?
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 @longieirl
- Understandability of the client prompt condition could be clearer but defer to the author
- Tests updated
Fix for #2128
validator
was invoked for both YUI and CLI. This PR removes the validator method and ensures thescp
prompt state is maintained correctly by adding a scp setter prompt to handle both flows, CLI and YUI.Example1;
ui5.yaml
Prompts;
Example2;
ui5.yaml
Prompts;
Example3;
Prompts;
Example4;
Example5;