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

[Data transfer] aborting prompts displays as error #15974

Merged
merged 3 commits into from
Mar 8, 2023
Merged

[Data transfer] aborting prompts displays as error #15974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9620ba
aborting prompts displays as error
innerdvations Mar 2, 2023
a1f34b3
cancelling prompts is treated as error
innerdvations Mar 2, 2023
606a88e
add jsdoc
innerdvations Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/core/strapi/bin/strapi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ program
},
]);
if (!answers.fromToken?.length) {
exitWith(0, 'No token entered, aborting transfer.');
exitWith(1, 'No token entered, aborting transfer.');
}
thisCommand.opts().fromToken = answers.fromToken;
}
Expand All @@ -336,13 +336,14 @@ program
},
]);
if (!answers.toToken?.length) {
exitWith(0, 'No token entered, aborting transfer.');
exitWith(1, 'No token entered, aborting transfer.');
}
thisCommand.opts().toToken = answers.toToken;
}

await confirmMessage(
'The transfer will delete all data in the remote database and media files. Are you sure you want to proceed?'
'The transfer will delete all data in the remote database and media files. Are you sure you want to proceed?',
{ failMessage: 'Transfer process aborted' }
)(thisCommand);
}
)
Expand Down Expand Up @@ -412,7 +413,7 @@ program
},
]);
if (!answers.key?.length) {
exitWith(0, 'No key entered, aborting import.');
exitWith(1, 'No key entered, aborting import.');
}
opts.key = answers.key;
}
Expand Down Expand Up @@ -450,7 +451,8 @@ program
.hook(
'preAction',
confirmMessage(
'The import will delete all data in your database and media files. Are you sure you want to proceed?'
'The import will delete all data in your database and media files. Are you sure you want to proceed?',
{ failMessage: 'Import process aborted' }
)
)
.action(getLocalScript('transfer/import'));
Expand Down
5 changes: 3 additions & 2 deletions packages/core/strapi/lib/commands/utils/commander.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ const promptEncryptionKey = async (thisCommand) => {
*
* @param {string} message The message to confirm with user
* @param {object} options Additional options
* @param {string|undefined} options.failMessage The message to display when prompt is not confirmed
*/
const confirmMessage = (message) => {
const confirmMessage = (message, { failMessage } = {}) => {
return async (command) => {
// if we have a force option, assume yes
const opts = command.opts();
Expand All @@ -116,7 +117,7 @@ const confirmMessage = (message) => {
},
]);
if (!answers.confirm) {
exitWith(0);
exitWith(1, failMessage);
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a default value for the fail message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended as a generic way to confirm a prompt, I'm not sure we could come up with text that wouldn't be more annoying to have to override every time.

I'm already not completely convinced on this being an error exit code, because if the user is specifically saying "no" it's not an error, it's working as intended. (and I at least want to keep the link between "red message" and "error" and not deviate from that)

Copy link
Member

@Convly Convly Mar 7, 2023 •
edited

Choose a reason for hiding this comment

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

I'm already not completely convinced on this being an error exit code
Yeah, this is a tricky one.

Here are some examples about exit codes for canceled commands
image

Copy link
Member

Choose a reason for hiding this comment

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

Not sure which one we should use though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm interpreting this right:
First example you cancelled during a prompt and got a 0.
Second example you cancelled while it was working and got a 1.
Third example was cancelled during a password prompt for sudo and got a 1.

I think my preference is for a 0/warning, but I'm not opposed to a 1/error, so I think we should just go with it since it makes it easier for anything reading the exit code to interpret it as "ok, there was an error so the transfer didn't succeed" and also lets the user know the same with red text.

}
};
};
Expand Down