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

fix: askx new -> change the representation layer for deployers #106

Merged
merged 5 commits into from
Apr 7, 2020
Merged

fix: askx new -> change the representation layer for deployers #106

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9564ce7
fix: askx new -> change the representation layer for deployers
Chih-Ying Apr 7, 2020
c58453c
fix: add skipUnwrap to 2 new complex body parameters
Apr 7, 2020
b6fd81f
fix: askx new -> change the representation layer for deployers
Chih-Ying Apr 7, 2020
749b3f8
Merge branch 'deployer-representation-fix' of github.com:alexa-labs/a…
Chih-Ying Apr 7, 2020
7e47997
fix: improve the way to map a deployer option name
Chih-Ying Apr 7, 2020
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
9 changes: 7 additions & 2 deletions lib/commands/v2new/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ function confirmUsingUnofficialTemplate(callback) {

function getDeploymentType(deployType, callback) {
const deployDelegateChoices = R.values(deployType).map(
(deployer) => `${deployer.NAME}\n ${chalk.gray(deployer.DESCRIPTION)}`
(deployer) => `${deployer.OPTION_NAME}\n ${chalk.gray(deployer.DESCRIPTION)}`
);
deployDelegateChoices.push(new inquirer.Separator());
deployDelegateChoices.push(SKIP_DEPLOY_DELEGATE_SELECTION);
Expand All @@ -116,7 +116,12 @@ function getDeploymentType(deployType, callback) {
pageSize: 30,
filter: input => input.replace(/\n.*/g, '')
}]).then((answer) => {
callback(null, answer.deployDelegate);
// eslint-disable-next-line array-callback-return
R.values(deployType).map((deployer) => {
if (answer.deployDelegate === deployer.OPTION_NAME) {
return callback(null, deployer.NAME);
}
});
Copy link
Contributor

@RonWang RonWang Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think if not understanding that the loop will only hit the target once, putting the callback inside a for loop looks not good. The eslint actually makes sense and we should not ignore. I think you can use try

callback(null, R.find(R.propEq('OPTION_NAME', 'AWS Lambda'))(R.values(deployType)).NAME)

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 solution is much better, it returns the first element of the list which matches the predicate, or undefined if no element matches.

}).catch((error) => {
callback(error);
});
Expand Down
3 changes: 3 additions & 0 deletions lib/utils/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@ module.exports.METRICS = {

module.exports.DEPLOYER_TYPE = {
HOSTED: {
OPTION_NAME: 'Alexa-hosted skills',
NAME: '@ask-cli/hosted-skill-deployer',
DESCRIPTION: 'Host your skill code by Alexa (free).'
},
CFN: {
OPTION_NAME: 'AWS with CloudFormation',
NAME: '@ask-cli/cfn-deployer',
DESCRIPTION: 'Host your skill code with AWS services and provision with AWS CloudFormation (requires AWS account)'
},
LAMBDA: {
OPTION_NAME: 'AWS Lambda',
NAME: '@ask-cli/lambda-deployer',
DESCRIPTION: 'Host your skill code on AWS Lambda (requires AWS account).'
}
Expand Down
15 changes: 9 additions & 6 deletions test/unit/commands/v2new/ui-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ describe('Commands new - UI test', () => {
const TEST_LANGUAGE = 'language';
const TEST_TEMPLATE_NAME = 'templateName';
const TEST_CONFIRMATION = 'confirmation';
const TEST_DEPLOY_DELEGATE = 'deployDelegate';
const TEST_DEPLOYMENT_OPTION_NAME = 'HOSTED_OPTION_NAME';
const TEST_DEPLOYMENT_NAME = 'HOSTED_NAME';
const TEST_TEMPLATES_MAP = {
template1: {
url: 'templateUrl1',
Expand All @@ -39,10 +40,12 @@ describe('Commands new - UI test', () => {
};
const TEST_DEPLOYMENT_MAP = {
HOSTED: {
NAME: 'HOSTED_NAME',
OPTION_NAME: TEST_DEPLOYMENT_OPTION_NAME,
NAME: TEST_DEPLOYMENT_NAME,
DESCRIPTION: 'HOSTED_DESCRIPTION'
},
CFN: {
OPTION_NAME: 'CFN_OPTION_NAME',
NAME: 'CFN_NAME',
DESCRIPTION: 'CFN_DESCRIPTION'
}
Expand All @@ -52,8 +55,8 @@ describe('Commands new - UI test', () => {
`template2\t\t ${chalk.gray('')}`
];
const TEST_DEPLOYMENT_CHOICES_WITH_SEP = [
`HOSTED_NAME\n ${chalk.gray('HOSTED_DESCRIPTION')}`,
`CFN_NAME\n ${chalk.gray('CFN_DESCRIPTION')}`,
`${TEST_DEPLOYMENT_OPTION_NAME}\n ${chalk.gray('HOSTED_DESCRIPTION')}`,
`CFN_OPTION_NAME\n ${chalk.gray('CFN_DESCRIPTION')}`,
new inquirer.Separator(),
ui.SKIP_DEPLOY_DELEGATE_SELECTION
];
Expand Down Expand Up @@ -371,7 +374,7 @@ describe('Commands new - UI test', () => {

it('| confirmation entered by user is retrieved correctly', (done) => {
// setup
inquirer.prompt.resolves({ deployDelegate: TEST_DEPLOY_DELEGATE });
inquirer.prompt.resolves({ deployDelegate: TEST_DEPLOYMENT_OPTION_NAME });
// call
ui.getDeploymentType(TEST_DEPLOYMENT_MAP, (err, response) => {
// verify
Expand All @@ -382,7 +385,7 @@ describe('Commands new - UI test', () => {
});
expect(inquirer.prompt.args[0][0][0].filter('a \n \n b')).equal('a ');
expect(err).equal(null);
expect(response).equal(TEST_DEPLOY_DELEGATE);
expect(response).equal(TEST_DEPLOYMENT_NAME);
done();
});
});
Expand Down