fix(appsync): resolver unable to set pipelineConfig #9093

BryanPan342 · 2020-07-16T01:03:34Z

[ISSUE]
pipelineConfig was be labeled as undefined while using the Resolver class instead of createResolver. Also, no way to set kind parameter for resolvers, so implemented that as well.

[APPROACH]
Created a property that takes pipelineConfig for AppSync functions.

[NOTE]
pipelineConfig takes a string array for the name of AppSync Functions not Lambda Functions

Fixes #6923

BREAKING CHANGE: pipelineConfig is now an array of string instead of CfnResolver.PipelineConfigProperty for usability.

appsync: pipelineConfig parameter takes in string []

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

mergify · 2020-07-16T01:04:10Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

mergify · 2020-07-16T16:43:49Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…342/aws-cdk into appsysnc-pipelineConfig

Pull request has been modified.

shivlaks · 2020-07-17T00:29:47Z

packages/@aws-cdk/aws-appsync/lib/resolver.ts

+  /**
+   * type of resolver
+   *
+   * @default - UNIT resolver, single data source


Suggested change

* @default - UNIT resolver, single data source

* @default ResolverType.UNIT

shivlaks · 2020-07-17T00:30:47Z

packages/@aws-cdk/aws-appsync/lib/resolver.ts

  /**
   * configuration of the pipeline resolver
   *
-   * @default - create a UNIT resolver
+   * @default - No pipelineConfig


Suggested change

* @default - No pipelineConfig

* @default - no pipeline resolver configuration

shivlaks · 2020-07-17T00:31:03Z

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

@@ -19,3 +19,27 @@ test('should not throw an Error', () => {
  // Then
  expect(when).not.toThrow();
 });
+
+test('should not throw an Error', () => {
+  // Given


Suggested change

// Given

// GIVEN

shivlaks · 2020-07-17T00:31:12Z

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

+  // Given
+  const stack = new cdk.Stack();
+
+  // When


Suggested change

// When

// WHEN

shivlaks · 2020-07-17T00:31:23Z

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

+    });
+  };
+
+  // Then


Suggested change

// Then

// WHEN

shivlaks · 2020-07-17T00:32:46Z

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

@@ -19,3 +19,27 @@ test('should not throw an Error', () => {
  // Then
  expect(when).not.toThrow();
 });
+
+test('should not throw an Error', () => {


how about a more descriptive name that contain the conditions under test

shivlaks · 2020-07-17T00:34:44Z

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

+  };
+
+  // Then
+  expect(when).not.toThrow();


can we verify the expected cloudformation template?

Pull request has been modified.

shivlaks

can we also add a README example to cover usage of the pipeline config?

shivlaks · 2020-07-20T15:50:40Z

packages/@aws-cdk/aws-appsync/lib/resolver.ts

   */
-  readonly pipelineConfig?: CfnResolver.PipelineConfigProperty | IResolvable;
+  readonly pipelineConfig?: string[];


can this be more strongly typed? or is there a good reason why strings would be ideal?

this motivation for this is not covered in the commit body, can we add that?
it's especially important since this is a breaking change (also needs mention in commit body)

mmmm originally the type would have been this:

{ functions: [...] }

where the functions parameter just took a string [] anyways, so changed it to just a string [] after following Mitch's suggestion.

also is this a breaking change?

the pipelineConfig prop doesn't actually do anything

do we have an issue for pipelineConfig doing nothing?

consider this:

user has a CDK application where they have specified a pipelineConfig of type PipelineConfigProperty

after this change, when they "upgrade" and just build.. it will fail saying the type is not a match. They have not changed anything about their application other than upgrade, but their build is now failing.

I think it would break which is why I'd consider this to be a breaking change. what do you think?

@shivlaks issue is here #6923

Flattening out the object is a convenience I think since it's the only property. Technically cfn could add more optional properties in the future and we would have to handle those but that happens either way.

I think the more strongly typed could be 'IFunction[]'. @BryanPan342 does that make sense to you?

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

shivlaks

LGTM. added a do-not-merge so @MrArnoldPalmer can also have a look

MrArnoldPalmer

I think typing to 'IFunction[]' for pipelineConfig probably makes sense. We can also change the property name to 'pipelineFunctions' to make it more descriptive and different to the L1 prop.

BryanPan342 · 2020-07-24T00:31:08Z

I think typing to 'IFunction[]' for pipelineConfig probably makes sense. We can also change the property name to 'pipelineFunctions' to make it more descriptive and different to the L1 prop.

@MrArnoldPalmer so pipelineConfig doesn't take Lambda Functions. It takes AppSync's own type of Functions that use VTL to perform operations.

See documentation on pipelineConfig here.

See documentation on functionConfiguration here.

See documentation on appsync:CreateFunction here.

I opened up an issue #9092 for a feature request for AppSync function creation. Currently you can make an L1 version of it. But the pipelineConfg prop only takes a name of a AppSync Function.

mergify · 2020-07-24T01:20:28Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2020-07-24T16:48:29Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-07-24T17:08:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 3acd9f0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-07-24T17:08:37Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

**[ISSUE]** `pipelineConfig` was be labeled as `undefined` while using the `Resolver` class instead of `createResolver`. Also, no way to set `kind` parameter for resolvers, so implemented that as well. **[APPROACH]** Created a property that takes `pipelineConfig` for `AppSync` functions. **[NOTE]** `pipelineConfig` takes a string array for the name of `AppSync Functions` not `Lambda Functions` Fixes aws#6923 BREAKING CHANGE: `pipelineConfig` is now an array of `string` instead of `CfnResolver.PipelineConfigProperty` for usability. - **appsync**: `pipelineConfig` parameter takes in `string []` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

bug(appsync): tenative changes that prevent fail with pipelineConfg

33bea27

BryanPan342 self-assigned this Jul 16, 2020

fix linter

4f939ac

BryanPan342 requested review from MrArnoldPalmer and shivlaks July 16, 2020 01:08

BryanPan342 added the @aws-cdk/aws-appsync Related to AWS AppSync label Jul 16, 2020

BryanPan342 changed the title ~~bug(appsync): tentative changes that prevent fail with pipelineConfig~~ fix(appsync): tentative changes that prevent fail with pipelineConfig Jul 16, 2020

MrArnoldPalmer previously approved these changes Jul 16, 2020

View reviewed changes

mergify bot and others added 3 commits July 16, 2020 16:44

Merge branch 'master' into appsysnc-pipelineConfig

6f39337

appeasing pr linter

7b561b9

Merge branch 'appsysnc-pipelineConfig' of https://github.com/BryanPan…

1ed6768

…342/aws-cdk into appsysnc-pipelineConfig

address bugs

835278a

BryanPan342 changed the title ~~fix(appsync): tentative changes that prevent fail with pipelineConfig~~ fix(appsync): resolver unable to set pipelineConfig Jul 17, 2020

shivlaks previously requested changes Jul 17, 2020

View reviewed changes

refactor + add cases

4c88550

BryanPan342 requested a review from shivlaks July 17, 2020 16:25

shivlaks suggested changes Jul 20, 2020

View reviewed changes

BryanPan342 added pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. and removed pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. labels Jul 20, 2020

BryanPan342 requested review from shivlaks and MrArnoldPalmer July 20, 2020 16:32

shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Jul 23, 2020

shivlaks previously approved these changes Jul 23, 2020

View reviewed changes

MrArnoldPalmer suggested changes Jul 23, 2020

View reviewed changes

MrArnoldPalmer previously approved these changes Jul 24, 2020

View reviewed changes

BryanPan342 removed the pr/do-not-merge This PR should not be merged at this time. label Jul 24, 2020

mergify bot and others added 2 commits July 24, 2020 01:21

Merge branch 'master' into appsysnc-pipelineConfig

ce061f3

yay fixed merge conflicts

f0c7f5c

BryanPan342 requested a review from shivlaks July 24, 2020 16:45

MrArnoldPalmer approved these changes Jul 24, 2020

View reviewed changes

Merge branch 'master' into appsysnc-pipelineConfig

3acd9f0

mergify bot merged commit dac9bb3 into aws:master Jul 24, 2020

BryanPan342 deleted the appsysnc-pipelineConfig branch September 8, 2020 18:36

	* @default - UNIT resolver, single data source
	* @default ResolverType.UNIT

	* @default - No pipelineConfig
	* @default - no pipeline resolver configuration

+                  });
+                };
+                // Then

fix(appsync): resolver unable to set pipelineConfig #9093

fix(appsync): resolver unable to set pipelineConfig #9093

Uh oh!

Conversation

BryanPan342 commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Jul 16, 2020

Uh oh!

mergify bot commented Jul 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shivlaks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shivlaks left a comment

Choose a reason for hiding this comment

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

BryanPan342 commented Jul 24, 2020

Uh oh!

mergify bot commented Jul 24, 2020

Uh oh!

mergify bot commented Jul 24, 2020

Uh oh!

aws-cdk-automation commented Jul 24, 2020

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 24, 2020

Uh oh!

Uh oh!

BryanPan342 commented Jul 16, 2020 •

edited

Loading