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

feat: add new flag support for config commands to force profile input flag #272

Merged
merged 6 commits into from
Aug 2, 2021
Merged
Show file tree
Hide file tree
Changes from 5 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
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,13 @@ Before you submit your pull request consider the following guidelines:
* `npm uninstall -g twilio-cli`
* `git clone https://github.com/twilio/twilio-cli-core.git`
* `cd twilio-cli-core`
* ``make clean install`
* `make clean install`
* `cd ..`
* `git clone https://github.com/twilio/twilio-cli.git`
* `cd twilio-cli`
* In `package.json` replace `"@twilio/cli-core": "<Version Number>"` with `"@twilio/cli-core": "file:../twilio-cli-core"`
* `make clean install`
* Test that everything is wired up correctuly with `./bin/run`
* Test that everything is wired up correctly with `./bin/run`
Comment on lines +96 to +102
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇


* Understanding the code base:
* The Twilio CLI utilizes the Open CLI Framework ([oclif](https://oclif.io/)). It may be useful to familiarize yourself with it, in particular, the [multi-command CLI](https://oclif.io/docs/multi).
Expand Down
7 changes: 4 additions & 3 deletions src/commands/config/list.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
const { BaseCommand } = require('@twilio/cli-core').baseCommands;

const { availableConfigs, getFromEnvironment } = require('../../services/config-utility');
const { getFromEnvironment } = require('../../services/config-utility');

class ConfigList extends BaseCommand {
async run() {
await super.run();
const configData = [];
availableConfigs.forEach((config) => {
Object.keys(this.userConfig).forEach((config) => {
let configEnvValue = getFromEnvironment(config);
if (configEnvValue) {
configEnvValue += '[env]';
}

configData.push({
configName: config,
value: configEnvValue || this.userConfig[config],
value: JSON.stringify(configEnvValue || this.userConfig[config]),
});
});
this.output(configData);
Expand Down
14 changes: 12 additions & 2 deletions src/commands/config/set.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const { TwilioCliError } = require('@twilio/cli-core/src/services/error');
const { BaseCommand } = require('@twilio/cli-core').baseCommands;
const { flags } = require('@oclif/command');
const { camelCase } = require('@twilio/cli-core').services.namingConventions;

const { availableConfigs, getFromEnvironment } = require('../../services/config-utility');

Expand All @@ -10,15 +11,16 @@ class ConfigSet extends BaseCommand {
let isError = true;
let isUserConfigUpdated = false;
for (const flag of availableConfigs) {
const configProperty = camelCase(flag);
if (this.flags[flag] !== undefined) {
isError = false;
this.preWarnings(flag);
if (this.flags[flag] === '') {
isUserConfigUpdated = await this.removeConfig(flag, isUserConfigUpdated);
continue;
}
if (await this.isOverwrite(flag)) {
this.userConfig[flag] = this.userConfig.sanitize(this.flags[flag]);
if (await this.isOverwrite(configProperty)) {
this.userConfig[configProperty] = this.sanitizeFlag(this.flags[flag]);
isUserConfigUpdated = true;
}
}
Expand All @@ -33,6 +35,10 @@ class ConfigSet extends BaseCommand {
}
}

sanitizeFlag(flag) {
return typeof flag === 'string' ? this.userConfig.sanitize(flag) : flag;
}

Comment on lines +38 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

🆙

preWarnings(flag) {
const flagEnvValue = getFromEnvironment(flag);
if (flagEnvValue) {
Expand Down Expand Up @@ -84,6 +90,10 @@ ConfigSet.flags = {
char: 'e',
description: 'Sets an Edge configuration.',
}),
'require-profile-input': flags.boolean({
description: 'Whether the profile flag for Twilio CLI commands is required or not.',
allowNo: true,
}),
Comment on lines +93 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q] Do we require default to false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would change the functionality for the require-profile-input flag. Right now the we only need to pass the flag when we try to update it. Consider the scenario when the flag is set. Now if the default is set to false, whenever the config:set command is called for some other property say edge. it would try to override the require-profile-input flag to false, even if the flag is not passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, makes sense. :)

...BaseCommand.flags, // To add the same flags as BaseCommand
};
module.exports = ConfigSet;
2 changes: 1 addition & 1 deletion src/services/config-utility.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const availableConfigs = ['edge'];
const availableConfigs = ['edge', 'require-profile-input'];
function getFromEnvironment(config) {
const configEnv = `TWILIO_${config.toUpperCase()}`;
return process.env[configEnv];
Expand Down
60 changes: 42 additions & 18 deletions test/commands/config/list.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,45 +6,69 @@ const ConfigList = require('../../../src/commands/config/list');
describe('commands', () => {
describe('config', () => {
describe('list', () => {
const listConfig = ({ addEdge = '' } = {}) =>
const listConfig = ({ configProperty, configPropertyValue } = {}) =>
test
.do((ctx) => {
ctx.userConfig = new ConfigData();
if (addEdge) {
ctx.userConfig.edge = addEdge;
if (configProperty) {
ctx.userConfig[configProperty] = configPropertyValue;
}
})
.twilioCliEnv(Config)
.twilioCreateCommand(ConfigList, [])
.stdout()
.stderr();

listConfig({ addEdge: 'testEdge' })
// parsing the stdout to a key value pair
const parseOutputToMap = (output) => {
const propertyWithValue = output.split('\n');
const propertyMap = new Map();
propertyWithValue.forEach((value) => {
const entry = value.trim().split(/\s{2,}/);
propertyMap.set(entry[0], entry[1]);
});
return propertyMap;
};
listConfig({ configProperty: 'edge', configPropertyValue: 'testEdge' })
.do((ctx) => ctx.testCmd.run())
.it('runs config:list, should list config variables', (ctx) => {
expect(ctx.stdout).to.contain('Config Name');
expect(ctx.stdout).to.contain('Value');
expect(ctx.stdout).to.contain('testEdge');
expect(ctx.stdout).to.contain('edge');
const configListOutput = parseOutputToMap(ctx.stdout);
expect(configListOutput.get('edge')).is.equal('"testEdge"');
});
listConfig({ addEdge: 'testEdge' })
listConfig({ configProperty: 'edge', configPropertyValue: 'testEdge' })
.do((ctx) => {
process.env.TWILIO_EDGE = 'fakeEdge';
return ctx.testCmd.run();
})
.it('runs config:list, should prioritize environment if both environment and config edge set', (ctx) => {
expect(ctx.stdout).to.contain('Config Name');
expect(ctx.stdout).to.contain('Value');
expect(ctx.stdout).to.contain('fakeEdge[env]');
expect(ctx.stdout).to.contain('edge');
const configListOutput = parseOutputToMap(ctx.stdout);
expect(configListOutput.get('edge')).is.not.undefined;
expect(configListOutput.get('edge')).is.equal('"fakeEdge[env]"');
});
listConfig({})
.do((ctx) => ctx.testCmd.run())
.it('runs config:list, should list empty config properties are not set', (ctx) => {
const configListOutput = parseOutputToMap(ctx.stdout);
expect(configListOutput.get('edge')).is.undefined;
expect(configListOutput.get('requireProfileInput')).is.undefined;
expect(configListOutput.get('email')).is.equal('{}');
expect(configListOutput.get('projects')).is.equal('[]');
expect(configListOutput.get('activeProfile')).is.equal('null');
expect(configListOutput.get('profiles')).is.equal('{}');
});
listConfig({ configProperty: 'requireProfileInput', configPropertyValue: true })
.do((ctx) => ctx.testCmd.run())
.it('runs config:list, should list requireProfileInput as set', (ctx) => {
const configListOutput = parseOutputToMap(ctx.stdout);
expect(configListOutput.get('requireProfileInput')).is.not.undefined;
expect(configListOutput.get('requireProfileInput')).is.equal('true');
});
listConfig({})
.do((ctx) => ctx.testCmd.run())
.it('runs config:list, should list empty as edge is not set', (ctx) => {
expect(ctx.stdout).to.contain('Config Name');
expect(ctx.stdout).to.contain('Value');
expect(ctx.stdout).to.contain('');
expect(ctx.stdout).to.contain('edge');
.it('runs config:list, should list all config properties in userConfig', (ctx) => {
Object.keys(new ConfigData()).forEach((configProperty) => {
expect(ctx.stdout).to.contain(configProperty);
});
});
});
});
Expand Down
147 changes: 91 additions & 56 deletions test/commands/config/set.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,79 +7,114 @@ const ConfigSet = require('../../../src/commands/config/set');
describe('commands', () => {
describe('config', () => {
describe('set', () => {
const createConfig = (commandArgs = [], { addEdge = '', updateEdge = true, removeEdge = true } = {}) =>
const createConfig = (
commandArgs = [],
{ configProperty = '', configPropertyValue, updateConfigProperty = true, removeConfigProperty = true } = {},
) =>
test
.do((ctx) => {
ctx.userConfig = new ConfigData();
if (addEdge) {
ctx.userConfig.edge = addEdge;
if (configProperty) {
ctx.userConfig[configProperty] = configPropertyValue;
}
})
.twilioCliEnv(Config)
.twilioCreateCommand(ConfigSet, commandArgs)
.do((ctx) => {
const fakePrompt = sinon.stub();
fakePrompt.onFirstCall().resolves({
Overwrite: updateEdge,
Remove: removeEdge,
Overwrite: updateConfigProperty,
Remove: removeConfigProperty,
});
ctx.testCmd.inquirer.prompt = fakePrompt;
})
.stdout()
.stderr();
describe('edge', () => {
createConfig(['--edge=createEdge'])
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge=createEdge', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge=updateEdge'], { configProperty: 'edge', configPropertyValue: 'createEdge' })
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge=updateEdge', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('updateEdge');
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge=updateEdge'], {
configProperty: 'edge',
configPropertyValue: 'createEdge',
updateConfigProperty: false,
})
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge=updateEdge with overwrite as false', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.not.contain('There is an environment variable already set');
});
createConfig(['--edge=createEdge'], {})
.do((ctx) => {
process.env.TWILIO_EDGE = 'envEdge';
return ctx.testCmd.run();
})
.it('runs config:set --edge=createEdge, will show warning if environment variable exists', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.contain('There is an environment variable already set for edge : envEdge');
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge='], { configProperty: 'edge', configPropertyValue: 'createEdge' })
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge= ,should remove the edge from configuration', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.be.undefined;
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge='], {
configProperty: 'edge',
configPropertyValue: 'createEdge',
removeConfigProperty: false,
})
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge= ,should not remove the edge from configuration', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.not.contain('configuration saved');
});
createConfig(['--edge='], {})
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge= ,should throw error as there is no existing edge', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.be.undefined;
expect(ctx.stderr).to.contain('There is no existing edge to remove.');
});
});

createConfig(['--edge=createEdge'])
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge=createEdge', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge=updateEdge'], { addEdge: 'createEdge' })
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge=updateEdge', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('updateEdge');
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge=updateEdge'], { addEdge: 'createEdge', updateEdge: false })
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge=updateEdge with overwrite as false', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.not.contain('There is an environment variable already set');
});
createConfig(['--edge=createEdge'], {})
.do((ctx) => {
process.env.TWILIO_EDGE = 'envEdge';
return ctx.testCmd.run();
describe('require-profile-input', () => {
createConfig(['--require-profile-input'])
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --require-profile-input', (ctx) => {
expect(ctx.testCmd.userConfig.requireProfileInput).to.be.true;
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--no-require-profile-input'], {
configProperty: 'requireProfileInput',
configPropertyValue: true,
})
.it('runs config:set --edge=createEdge, will show warning if environment variable exists', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.contain('There is an environment variable already set for edge : envEdge');
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge='], { addEdge: 'createEdge' })
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge= ,should remove the edge from configuration', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.be.undefined;
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--edge='], { addEdge: 'createEdge', removeEdge: false })
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge= ,should not remove the edge from configuration', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.contain('createEdge');
expect(ctx.stderr).to.not.contain('configuration saved');
});
createConfig(['--edge='], {})
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge= ,should throw error as there is no existing edge', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.be.undefined;
expect(ctx.stderr).to.contain('There is no existing edge to remove.');
});
createConfig(['--edge='], {})
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --edge= ,should throw error as there is no existing edge', (ctx) => {
expect(ctx.testCmd.userConfig.edge).to.be.undefined;
expect(ctx.stderr).to.contain('There is no existing edge to remove.');
});
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --no-require-profile-input', (ctx) => {
expect(ctx.testCmd.userConfig.requireProfileInput).to.be.false;
expect(ctx.stderr).to.contain('configuration saved');
});
createConfig(['--no-require-profile-input'], {
configProperty: 'requireProfileInput',
configPropertyValue: true,
updateConfigProperty: false,
})
.do((ctx) => ctx.testCmd.run())
.it('runs config:set --no-require-profile-input with confirmation overwrite as false', (ctx) => {
expect(ctx.testCmd.userConfig.requireProfileInput).to.be.true;
expect(ctx.stderr).to.not.contain('There is an environment variable already set');
});
});

createConfig([], {})
.do((ctx) => ctx.testCmd.run())
.catch(/No configuration is added to set. Run "twilio configs:set --help" to see how to set a configurations./)
Expand Down