feat(alexa-ask): Add deploy action for Alexa #1613

hoegertn · 2019-01-25T23:01:45Z

Pull Request Checklist

Testing
- Unit test added
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

sam-goodwin · 2019-01-26T18:10:42Z

The build is failing, but this looks great! I think the build was broken when you submitted this and was recently fixed by this PR: #1616

A re-run will likely pass.

RomainMuller · 2019-01-28T10:05:57Z

packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-alexa-deploy.ts

+
+const deployStage = new codepipeline.Stage(pipeline, 'Deploy', { pipeline });
+
+const clientId = new cdk.SecretParameter(stack, 'AlexaClientId', {ssmParameter: '/Alexa/ClientId'});


That integration test won't deploy unless the SSM Parameters exist, right? It'd be nice to somehow swap those out for phony values or locally-created entities instead, so the test can run without any pre-requisite set-up.

Good point. I changed it.

skinny85

Looks great @hoegertn , thanks so much for another contribution!

I have a few minor comments/questions, but nothing serious.

skinny85 · 2019-01-29T00:39:22Z

packages/@aws-cdk/alexa-ask/README.md

+
+const clientId = new SecretParameter(this, 'AlexaClientId', {ssmParameter: '/Alexa/ClientId'});
+const clientSecret = new SecretParameter(this, 'AlexaClientSecret', {ssmParameter: '/Alexa/ClientSecret'});
+const refreshToken = new SecretParameter(this, 'AlexaRefreshToken', {ssmParameter: '/Alexa/RefreshToken'});


I don't think you have to repeat these here - they're the same as above, so they don't really bring new information. I would just delete these.

skinny85 · 2019-01-29T00:40:48Z

packages/@aws-cdk/alexa-ask/lib/pipeline-actions.ts

+  /**
+   * The source artifact containing the voice model and skill manifest
+   */
+  inputArtifact: codepipeline.Artifact;


Can be optional, inputArtifact?: ....

skinny85 · 2019-01-29T00:42:00Z

packages/@aws-cdk/alexa-ask/lib/pipeline-actions.ts

+        minInputs: 1,
+        maxInputs: 2,
+        minOutputs: 0,
+        maxOutputs: 1,


Can this Action really have outputs? Deployment Actions almost never do... If 'yes', what is that output?

skinny85 · 2019-01-29T00:43:19Z

packages/@aws-cdk/alexa-ask/lib/pipeline-actions.ts

+  /**
+   * An optional artifact containing overrides for the skill manifest
+   */
+  overrideArtifact?: codepipeline.Artifact;


I'm not in love with this name. How about something like skillManifestArtifact?

I chose the name because Alexa calls these parameters overrides. It is not the Skill manifest but a JSON containing overrides for the manifest provided as inputArtifact. Normally this is an output of CFN with the Lambda ARN.

OK. Can you change this to parameterOverridesArtifact then? Thanks!

skinny85 · 2019-01-29T00:45:41Z

packages/@aws-cdk/alexa-ask/lib/pipeline-actions.ts

+        RefreshToken: props.refreshToken,
+        SkillId: props.skillId,
+      },
+      inputArtifact: props.inputArtifact,


Use TypeScript's spread operator here instead:

super(scope, id, { artifactBounds: { minInputs: 1, maxInputs: 2, minOutputs: 0, maxOutputs: 1, }, owner: 'ThirdParty', provider: 'AlexaSkillsKit', configuration: { ClientId: props.clientId, ClientSecret: props.clientSecret, RefreshToken: props.refreshToken, SkillId: props.skillId, }, ...props, });

It's usually safer to use the spread operator up-front, unless you are intentionally allowing whoever provided props to override the other keys?

I've taken the liberty to move the splat up in the latest revision - that feels a lot safer to me.

skinny85 · 2019-01-29T00:46:28Z

packages/@aws-cdk/alexa-ask/package.json

@@ -61,12 +61,14 @@
    "pkglint": "^0.22.0"
  },
  "dependencies": {
-    "@aws-cdk/cdk": "^0.22.0"
+    "@aws-cdk/cdk": "^0.22.0",
+    "@aws-cdk/aws-codepipeline-api": "^0.22.0"


This should be above the cdk dependency (we try to keep them in alphabetical order).

skinny85 · 2019-01-29T00:46:38Z

packages/@aws-cdk/alexa-ask/package.json

  },
  "peerDependencies": {
-    "@aws-cdk/cdk": "^0.22.0"
+    "@aws-cdk/cdk": "^0.22.0",
+    "@aws-cdk/aws-codepipeline-api": "^0.22.0"


Same here as above (order).

skinny85 · 2019-01-29T00:47:24Z

packages/@aws-cdk/aws-codepipeline/package.json

@@ -68,6 +68,7 @@
    "@aws-cdk/aws-codedeploy": "^0.22.0",
    "@aws-cdk/aws-ecr": "^0.22.0",
    "@aws-cdk/aws-lambda": "^0.22.0",
+    "@aws-cdk/alexa-ask": "^0.22.0",


This should be the first dependency (alphabetical order).

skinny85 · 2019-01-29T00:48:51Z

packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-alexa-deploy.ts

+  runOrder: 1,
+  inputArtifact: sourceAction.outputArtifact,
+  clientId: new Secret('clientId'),
+  clientSecret: new Secret('clientSecret'),


Use the cdk import for these: new cdk.Secret(...).

hoegertn · 2019-01-29T06:43:33Z

Thanks for the feedback. Should all be fixed now.

skinny85

There's still the question of the whether this can produce outputs that I asked in my previous review.

I'm asking, as that actually influences the API of the Action. If an output can be produced, we might have to add an optional property to its construction props, in order to be able to name the output Artifact, and also an optional field on the Action itself that allows access to it.

skinny85 · 2019-01-30T23:16:08Z

packages/@aws-cdk/alexa-ask/lib/pipeline-actions.ts

+  /**
+   * An optional artifact containing overrides for the skill manifest
+   */
+  overrideArtifact?: codepipeline.Artifact;


OK. Can you change this to parameterOverridesArtifact then? Thanks!

hoegertn · 2019-01-31T07:26:28Z

@skinny85 I changed it to parameterOverridesArtifact. The outputs are now 0 so this should answer your question.

skinny85

Awesome, LGTM! Thanks a lot @hoegertn!

hoegertn · 2019-02-01T10:40:34Z

Thanks, for the review and the comments.

Any idea when the next release will be and if this change will make it into the release? I am talking at the Berlin Summit about CI/CD for Alexa Skills and it would be great to be able to use it.

RomainMuller · 2019-02-01T10:43:29Z

@hoegertn no promises, but I'm pretty confident this will be part of the next release, which I am hoping will happen early next week.

hoegertn requested review from RomainMuller, skinny85 and a team as code owners January 25, 2019 23:01

hoegertn mentioned this pull request Jan 25, 2019

Support Deploy to Alexa Action #1586

Closed

RomainMuller reviewed Jan 28, 2019

View reviewed changes

skinny85 suggested changes Jan 29, 2019

View reviewed changes

skinny85 reviewed Jan 30, 2019

View reviewed changes

hoegertn added 4 commits January 31, 2019 08:22

feat(alexa-ask): Add deploy action for Alexa

63abd8d

feat(alexa-ask): Add deploy action for Alexa

5668456

PR feedback

3a0990f

rename parameterOverridesArtifact

68a4222

skinny85 approved these changes Jan 31, 2019

View reviewed changes

Moved spread up to the front

9dcc6a7

RomainMuller approved these changes Feb 1, 2019

View reviewed changes

RomainMuller merged commit 0deea61 into aws:master Feb 1, 2019

hoegertn deleted the alexa-deploy branch September 27, 2019 07:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(alexa-ask): Add deploy action for Alexa #1613

feat(alexa-ask): Add deploy action for Alexa #1613

hoegertn commented Jan 25, 2019

sam-goodwin commented Jan 26, 2019

RomainMuller Jan 28, 2019

hoegertn Jan 28, 2019

skinny85 left a comment

skinny85 Jan 29, 2019

skinny85 Jan 29, 2019

skinny85 Jan 29, 2019

skinny85 Jan 29, 2019

hoegertn Jan 29, 2019

skinny85 Jan 30, 2019

skinny85 Jan 29, 2019

RomainMuller Feb 1, 2019

RomainMuller Feb 1, 2019

skinny85 Jan 29, 2019

skinny85 Jan 29, 2019

skinny85 Jan 29, 2019

skinny85 Jan 29, 2019

hoegertn commented Jan 29, 2019

skinny85 left a comment •

edited

Loading

skinny85 Jan 30, 2019

hoegertn commented Jan 31, 2019

skinny85 left a comment

hoegertn commented Feb 1, 2019

RomainMuller commented Feb 1, 2019


		const deployStage = new codepipeline.Stage(pipeline, 'Deploy', { pipeline });

		const clientId = new cdk.SecretParameter(stack, 'AlexaClientId', {ssmParameter: '/Alexa/ClientId'});

feat(alexa-ask): Add deploy action for Alexa #1613

feat(alexa-ask): Add deploy action for Alexa #1613

Conversation

hoegertn commented Jan 25, 2019

Pull Request Checklist

sam-goodwin commented Jan 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoegertn commented Jan 29, 2019

skinny85 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoegertn commented Jan 31, 2019

skinny85 left a comment

Choose a reason for hiding this comment

hoegertn commented Feb 1, 2019

RomainMuller commented Feb 1, 2019

skinny85 left a comment •

edited

Loading