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

feat: add --json to compute add collaborator #469

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −5
Open

feat: add --json to compute add collaborator #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion command-snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
{
"command": "env:compute:collaborator:add",
"plugin": "@salesforce/plugin-functions",
"flags": ["heroku-user"],
"flags": ["heroku-user", "json"],
"alias": []
},
{
Expand Down
7 changes: 4 additions & 3 deletions src/commands/env/compute/collaborator/add.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ Messages.importMessagesDirectory(__dirname);
const messages = Messages.loadMessages('@salesforce/plugin-functions', 'env.compute.collaborator.add');

export default class ComputeCollaboratorAdd extends Command {
jheikes515 marked this conversation as resolved.
Show resolved Hide resolved
static enableJsonFlag = false;

static summary = messages.getMessage('summary');

static examples = messages.getMessages('examples');
Expand Down Expand Up @@ -64,7 +62,7 @@ export default class ComputeCollaboratorAdd extends Command {
}

if (error.message?.includes('404')) {
jheikes515 marked this conversation as resolved.
Show resolved Hide resolved
this.error(`${herokuColor.heroku(herokuUser)} does not exist.`);
this.error(`Couldn't find Heroku user ${herokuColor.heroku(herokuUser)}.`);
jheikes515 marked this conversation as resolved.
Show resolved Hide resolved
}

this.error(error.message);
Expand All @@ -74,5 +72,8 @@ export default class ComputeCollaboratorAdd extends Command {
this.log(
'For more information about attaching Heroku add-ons to your compute environments, run $ heroku addons:attach --help.'
);
return {
Copy link
Member

Choose a reason for hiding this comment

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

If the user includes the --json flag, the error output should be in json also.

Try adding in:

if (shouldExitNonZero) {
      this.exit(1);
    }

Copy link
Member

Choose a reason for hiding this comment

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

Example here

plugin-functions/src/commands/deploy/functions.ts

Line 188 in e63ff43

if (shouldExitNonZero) {

added: [`${herokuUser}`],
};
}
}
22 changes: 21 additions & 1 deletion test/commands/env/compute/collaborator/add.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ import herokuColor from '@heroku-cli/color';

const HEROKU_USER = '[email protected]';

const jsonSuccess = {
status: 0,
result: {
added: [HEROKU_USER],
},
warnings: [],
};

describe('sf env compute collaborator add', () => {
test
.stdout()
Expand All @@ -33,13 +41,25 @@ describe('sf env compute collaborator add', () => {
);
});

test
Copy link
Member

Choose a reason for hiding this comment

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

Also add in a failing testing for the --json flag

.stdout()
.stderr()
.nock('https://api.heroku.com', (api) => api.post('/salesforce-orgs/collaborators').reply(200, {}))
.command(['env:compute:collaborator:add', '-h', HEROKU_USER, '--json'])
jheikes515 marked this conversation as resolved.
Show resolved Hide resolved
.it('will show json output with success', (ctx) => {
const succJSON = JSON.parse(ctx.stdout);

expect(succJSON.status).to.deep.equal(jsonSuccess.status);
expect(succJSON.result).to.eql(jsonSuccess.result);
});

test
.stdout()
.stderr()
.nock('https://api.heroku.com', (api) => api.post('/salesforce-orgs/collaborators').reply(404, {}))
.command(['env:compute:collaborator:add', '-h', HEROKU_USER])
jheikes515 marked this conversation as resolved.
Show resolved Hide resolved
.catch((error) => {
expect(error.message).contains(`${herokuColor.heroku(HEROKU_USER)} does not exist.`);
expect(error.message).contains(`Couldn't find Heroku user ${herokuColor.heroku(HEROKU_USER)}.`);
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment about this change. I'm not sure it's needed.

})
.it('alerts user if user entered does not exist');
});