BREAKING CHANGE(cli): Port Logout Command #60
Conversation
| const cleanedBody = {}; | ||
| for (const k in requestBody) { | ||
| if (k.includes('zip_file')) { | ||
| cleanedBody[k] = replacementStr; |
There was a problem hiding this comment.
now this won't add "zip removed" to requests without zip files in the first place.
| const deleteFile = fileName => { | ||
| return new Promise(resolve => { | ||
| try { | ||
| fse.unlinkSync(fileName); |
There was a problem hiding this comment.
I'm not sure why we were wrapping a sync method in a promise (instead of using the async version or no promise at all). Went ahead and did "just plain sync".
| } catch (e) { | ||
| this.warn( | ||
| 'Deletion API request failed. If this is unexpected, rerun this command with `--debug` for more info.' | ||
| ); |
There was a problem hiding this comment.
When the server fails to delete the key and returns an error, should we just exit? I know this follows the original implementation, but maybe it makes more sense not to continue if an error happens.
Another issue I found is when I tested zapier logout with .zapierrc absent, the output formatting is off:
I think adding a this.endSpinner() before printing the warning message should fix the formatting issue.
There was a problem hiding this comment.
used a finally! good catch.
Add new logout command. Given that it includes the changes made in zapier/zapier-platform-cli#422, it's technically major since it changes logout behavior.
I don't anticipate people wanting the old "kill all keys" behavior back, but if they do, they'll be able to do the same thing in the UI. if we need to add it back here, we can add a flag.
Testing wise, I may try and do it with a bunch of mocks and just validate that a bad server response is fine and that an auth file is deleted (or ignored silently).