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

BREAKING CHANGE(cli): Port Logout Command #60

Merged
merged 4 commits into from
Aug 30, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/cli/src/commands/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ module.exports = {
invite: require('./invite'),
link: require('./link'),
login: require('./login'),
logout: require('./logout'),
logs: require('./logs'),
migrate: require('./migrate'),
promote: require('./promote'),
Expand Down
52 changes: 0 additions & 52 deletions packages/cli/src/commands/logout.js

This file was deleted.

56 changes: 28 additions & 28 deletions packages/cli/src/oclif/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ Some notes to help an ongoing project

## helpful links

* the docs are fairly useful: https://oclif.io/docs/commands
* base command source is great for seeing what's available: https://github.com/oclif/command/blob/master/src/command.ts
- the docs are fairly useful: https://oclif.io/docs/commands
- base command source is great for seeing what's available: https://github.com/oclif/command/blob/master/src/command.ts

## migrating a command

Expand All @@ -18,32 +18,32 @@ Some notes to help an ongoing project

### easy

* [ ] apps
* [ ] delete
* [ ] describe
* [ ] deprecate
* [ ] help
* [ ] history
* [ ] invitees
* [ ] link
* [ ] login
* [ ] logout
* [ ] logs
* [ ] migrate
* [ ] migrate
* [ ] register
* [ ] test
- [ ] apps
- [ ] delete
- [ ] describe
- [ ] deprecate
- [ ] help
- [ ] history
- [ ] invitees
- [ ] link
- [ ] login
- [x] logout
- [ ] logs
- [ ] migrate
- [ ] migrate
- [ ] register
- [ ] test

## hard

* [ ] build
* [ ] collaborate
* [ ] convert
* [ ] env
* [ ] invite
* [ ] promote
* [ ] push
* [ ] scaffold
* [ ] upload
* [ ] watch - remove?
* [ ] validate
- [ ] build
- [ ] collaborate
- [ ] convert
- [ ] env
- [ ] invite
- [ ] promote
- [ ] push
- [ ] scaffold
- [ ] upload
- [ ] watch - remove?
- [ ] validate
36 changes: 36 additions & 0 deletions packages/cli/src/oclif/commands/logout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
const BaseCommand = require('../ZapierBaseCommand');
const { buildFlags } = require('../buildFlags');

const { callAPI } = require('../../utils/api');
const { deleteFile } = require('../../utils/files');
const { AUTH_LOCATION, AUTH_LOCATION_RAW } = require('../../constants');

class LogoutCommand extends BaseCommand {
async perform() {
this.log(
'Preparing to deactivate local deploy key and reset local configs.'
);
this.startSpinner('Deactivating local deploy key');
try {
await callAPI('/keys', { method: 'DELETE', body: { single: true } });
} catch (e) {
this.warn(
'Deletion API request failed. If this is unexpected, rerun this command with `--debug` for more info.'
);
Copy link
Member

@eliangcs eliangcs Aug 30, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used a finally! good catch.

}
this.stopSpinner();

this.startSpinner(`Destroying \`${AUTH_LOCATION_RAW}\``);
const success = deleteFile(AUTH_LOCATION);
this.debug(`file deletion success?: ${success}`);
this.stopSpinner();

this.log('The active deploy key was deactivated');
}
}

LogoutCommand.flags = buildFlags({ opts: { format: true } });
LogoutCommand.examples = ['zapier logout'];
LogoutCommand.description = `Deactivates your acive deploy key and resets \`${AUTH_LOCATION_RAW}\`.`;

module.exports = LogoutCommand;
1 change: 1 addition & 0 deletions packages/cli/src/oclif/oCommands.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@

module.exports = {
init: require('./commands/init'),
logout: require('./commands/logout'),
versions: require('./commands/versions')
};
13 changes: 9 additions & 4 deletions packages/cli/src/utils/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,15 @@ const callAPI = (route, options, rawError = false) => {
debug(`>> ${requestOptions.method} ${requestOptions.url}`);
if (requestOptions.body) {
const replacementStr = 'raw zip removed in logs';
const cleanedBody = _.assign({}, JSON.parse(requestOptions.body), {
zip_file: replacementStr,
source_zip_file: replacementStr
});
const requestBody = JSON.parse(requestOptions.body);
const cleanedBody = {};
for (const k in requestBody) {
if (k.includes('zip_file')) {
cleanedBody[k] = replacementStr;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now this won't add "zip removed" to requests without zip files in the first place.

} else {
cleanedBody[k] = requestBody[k];
}
}
debug(`>> ${JSON.stringify(cleanedBody)}`);
}
debug(`<< ${res.status}`);
Expand Down
18 changes: 8 additions & 10 deletions packages/cli/src/utils/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,14 @@ const writeFile = (fileName, data) => {
return fse.writeFile(fixHome(fileName), data);
};

// Returns a promise that deletes the file.
const deleteFile = fileName => {
return new Promise(resolve => {
try {
fse.unlinkSync(fileName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 (err) {
resolve();
}
resolve();
});
// deletes a file, eats the error
const deleteFile = path => {
try {
fse.unlinkSync(path);
return true;
} catch (err) {
return false;
}
};

// Returns a promise that ensures a directory exists.
Expand Down