fix(appsync): strongly type `expires` prop in apiKeyConfig #9122

BryanPan342 · 2020-07-16T22:56:32Z

[ISSUE]
apiKeyConfig has prop expires that has unclear documentation/not strongly typed and is prone to user errors.

[APPROACH]
Force expires to take Expiration class from core and will be able to output api key configurations easily through Expiration static functions: after(...), fromString(...), atDate(...), atTimeStamp(...).

Fixes #8698

BREAKING CHANGE: force apiKeyConfig require a Expiration class instead of string

appsync: Parameter apiKeyConfig takes Expiration class instead of 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-16T22:57:21Z

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

iliapolo

@eladb @MrArnoldPalmer This mainly involves your are ownerships, have a look as well?

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

packages/@aws-cdk/core/lib/expires.ts

iliapolo · 2020-07-20T22:40:21Z

@BryanPan342 Note that breaking changes should be specified with a BREAKING CHANGE prefix in the PR body. Like so: #9016

packages/@aws-cdk/core/lib/expires.ts

packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts

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

eladb · 2020-07-26T09:29:04Z

packages/@aws-cdk/aws-appsync/test/appsync.auth.graphql

+}
+
+type Query {
+  getTests: [ test! ]


How is this related to this change?

just for the integrity test to see if the API Key works for query/mutation

eladb · 2020-07-26T09:29:15Z

packages/@aws-cdk/aws-appsync/test/integ.auth-apikey.ts

+  typeName: 'Query',
+  fieldName: 'getTests',
+  requestMappingTemplate: MappingTemplate.dynamoDbScanTable(),
+  responseMappingTemplate: MappingTemplate.dynamoDbResultList(),


How is this related to this change?

same as the comment above

just for the integrity test to see if the API Key works for query/mutation

@eladb o i see.. i just need to check if the apikey exist is that right?

i think its important to test the connection else i can just remove the integ test all together

BryanPan342 · 2020-07-28T19:20:15Z

@iliapolo

I was suggested to move the equivalent of asEpoch() into core by @eladb and I'm more inclined to go with that because of the suggestion to implement an expires?.lessThan(...) into the Expiration class.

If we add those functions to Expiration I dont see why we can't do the same for asEpoch:

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

Line 528 in 8803379

    
           const getEpoch = (d: Expires) => { return Math.floor( d.date.getTime() / 86400000) * 86400; };

wdyt?

Pull request has been modified.

into appsync-expires

eladb

Still feels like this PR includes two different changes: one is the introduction of the Expiration class in core and using it in various places and the second is the change to the appsync library and the apikey tests.

packages/@aws-cdk/core/lib/expiration.ts

eladb · 2020-09-02T07:00:47Z

packages/@aws-cdk/core/lib/expiration.ts

+  /**
+   * Expiration value as a string
+   */
+  public readonly value: string;


Why is this needed? Seems like this the UTC representation of the date, which can be obtained from x.date.toUTCString(), no?

s3-deployments uses the value property, I added the date property myself, but I wouldn't mind just removing the date property entirely.

eladb · 2020-09-02T07:01:18Z

packages/@aws-cdk/core/lib/expiration.ts

+   *
+   * @param s the string that represents date to expire at
+   */
+  public static fromString(s: string) { return new Expiration(s, new Date(s)); }


Why is this needed? Expiration.atDate(new Date(s)) is short enough, no?

It existed in the s3-deployment version and I don't have the domain knowledge to know if this is something that should keep maybe @iliapolo knows more about this

From a user's mental model, I think .date makes much more sense then .value. We can change s3-deployment to use .date.toUTCString() or whatever it needs.

BryanPan342 · 2020-09-02T17:48:46Z

Still feels like this PR includes two different changes: one is the introduction of the Expiration class in core and using it in various places and the second is the change to the appsync library and the apikey tests.

@eladb would it be better if I separated this PR?

eladb · 2020-09-02T18:20:39Z

Still feels like this PR includes two different changes: one is the introduction of the Expiration class in core and using it in various places and the second is the change to the appsync library and the apikey tests.

@eladb would it be better if I separated this PR?

Yes, I think it makes sense to separate.

BryanPan342 · 2020-09-04T20:27:12Z

@iliapolo @eladb separated to #10192

moved core and s3 changes to #10192

Move `Expires` class from s3-deployments to core. Rename to `Expiration` **BREAKING CHANGE**: s3-deployments property `expires` takes `cdk.Expiration` instead of `Expires` - **s3-deployments**: `BucketDeploymentProps.expires` now takes in type `cdk.Expiration` **Note**: PR separated from #9122 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

BryanPan342 · 2020-09-08T17:53:09Z

@MrArnoldPalmer @shivlaks got #10192 approved! Would love it you guys could take a pass at this today. I dont think there is anything too controversial here. Maybe the integ test is unnecessary?

mergify · 2020-09-09T22:04:19Z

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-09-09T22:29:49Z

AWS CodeBuild CI Report

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

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

mergify · 2020-09-09T22:30:21Z

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).

fix(appsync): strongly type `expires` prop in apiKeyConfig (aws#9122)

apiKeyConfig expire works using Expires from @aws-cdk/core

ae65030

BryanPan342 requested review from iliapolo, MrArnoldPalmer and shivlaks July 16, 2020 22:56

BryanPan342 self-assigned this Jul 16, 2020

BryanPan342 added @aws-cdk/aws-appsync Related to AWS AppSync @aws-cdk/aws-s3-deployment @aws-cdk/core Related to core CDK functionality labels Jul 16, 2020

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

BryanPan342 added 3 commits July 16, 2020 17:12

Merge branch 'master' into appsync-expires

3699019

remove unused variable

5a72efe

update integ

72b5999

iliapolo reviewed Jul 20, 2020

View reviewed changes

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts Show resolved Hide resolved

packages/@aws-cdk/core/lib/expires.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/core/lib/expires.ts Outdated Show resolved Hide resolved

address suggestions for expire

9397fe4

BryanPan342 requested a review from iliapolo July 21, 2020 00:14

BryanPan342 added 2 commits July 21, 2020 08:37

run integ

7ac8c88

adjust integ to set up for a year

8803379

BryanPan342 requested a review from eladb July 22, 2020 20:19

eladb previously requested changes Jul 26, 2020

View reviewed changes

move utility to core

25dc05f

BryanPan342 added 2 commits July 28, 2020 12:36

linter stop it

93b26be

linter fix

8d7d415

BryanPan342 requested a review from eladb July 28, 2020 19:45

BryanPan342 added 3 commits July 28, 2020 12:46

Merge branch 'master' into appsync-expires

d0b3632

fix bug in s3-deployments

0b1bf97

Merge branch 'appsync-expires' of https://github.com/BryanPan342/aws-cdk

924a543

into appsync-expires

BryanPan342 added 3 commits September 1, 2020 11:31

Merge remote-tracking branch 'upstream/master'

d9ebafb

Merge remote-tracking branch 'upstream/master'

64db87e

Merge remote-tracking branch 'upstream/master'

b3622b0

eladb previously requested changes Sep 2, 2020

View reviewed changes

BryanPan342 added 4 commits September 2, 2020 10:42

address suggestions

c61f436

Merge remote-tracking branch 'upstream/master'

4cdd244

Merge branch 'master' into appsync-expires

5070407

fix merge conflicts

859c14d

BryanPan342 added 2 commits September 3, 2020 13:57

Merge remote-tracking branch 'upstream/master'

6608704

Merge remote-tracking branch 'upstream/master'

f85ce81

BryanPan342 mentioned this pull request Sep 4, 2020

refactor(core): move expiration from s3-deployments into core #10192

Merged

BryanPan342 removed @aws-cdk/aws-s3-deployment @aws-cdk/core Related to core CDK functionality labels Sep 5, 2020

BryanPan342 added 3 commits September 8, 2020 09:39

Merge remote-tracking branch 'upstream/master'

e8cfc0c

Merge remote-tracking branch 'upstream/master'

1423bc6

Merge branch 'master' into appsync-expires

d38aa2a

MrArnoldPalmer approved these changes Sep 9, 2020

View reviewed changes

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

Merge branch 'master' into appsync-expires

7af4dab

mergify bot merged commit 287f808 into aws:master Sep 9, 2020

vrr-21 added a commit to vrr-21/aws-cdk that referenced this pull request Sep 9, 2020

Merge pull request #2 from aws/master

b38640d

fix(appsync): strongly type `expires` prop in apiKeyConfig (aws#9122)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(appsync): strongly type `expires` prop in apiKeyConfig #9122

fix(appsync): strongly type `expires` prop in apiKeyConfig #9122

BryanPan342 commented Jul 16, 2020 •

edited

Loading

mergify bot commented Jul 16, 2020

iliapolo left a comment

iliapolo commented Jul 20, 2020

eladb Jul 26, 2020

BryanPan342 Jul 27, 2020

eladb Jul 26, 2020

BryanPan342 Jul 27, 2020

BryanPan342 Jul 28, 2020

BryanPan342 Jul 28, 2020

BryanPan342 commented Jul 28, 2020

eladb left a comment

eladb Sep 2, 2020

BryanPan342 Sep 2, 2020

eladb Sep 2, 2020

BryanPan342 Sep 2, 2020

eladb Sep 2, 2020

BryanPan342 commented Sep 2, 2020 •

edited

Loading

eladb commented Sep 2, 2020

BryanPan342 commented Sep 4, 2020

BryanPan342 commented Sep 8, 2020

mergify bot commented Sep 9, 2020

aws-cdk-automation commented Sep 9, 2020

mergify bot commented Sep 9, 2020

fix(appsync): strongly type expires prop in apiKeyConfig #9122

fix(appsync): strongly type expires prop in apiKeyConfig #9122

Conversation

BryanPan342 commented Jul 16, 2020 • edited Loading

mergify bot commented Jul 16, 2020

iliapolo left a comment

Choose a reason for hiding this comment

iliapolo commented Jul 20, 2020

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

BryanPan342 commented Jul 28, 2020

eladb 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

BryanPan342 commented Sep 2, 2020 • edited Loading

eladb commented Sep 2, 2020

BryanPan342 commented Sep 4, 2020

BryanPan342 commented Sep 8, 2020

mergify bot commented Sep 9, 2020

aws-cdk-automation commented Sep 9, 2020

AWS CodeBuild CI Report

mergify bot commented Sep 9, 2020

fix(appsync): strongly type `expires` prop in apiKeyConfig #9122

fix(appsync): strongly type `expires` prop in apiKeyConfig #9122

BryanPan342 commented Jul 16, 2020 •

edited

Loading

BryanPan342 commented Sep 2, 2020 •

edited

Loading