feat(events,applicationautoscaling): schedule can be a token #13064

sneharathod · 2021-02-15T21:10:38Z

Fix for Issue: #9413

Testing:
Unit tests added for Schedule.rate used in ApplicationAutoScaling and Events.

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parents, which 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

gitpod-io · 2021-02-15T21:10:40Z

mergify · 2021-02-15T21:11:17Z

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

nija-at

Thanks for submitting this PR 😊 My comments are below.

Looks like the build and other PR checks are failing. Can you look into them?

As for the failing CodeBuild job, the build logs are available as a comment above in this PR.

nija-at · 2021-02-16T10:30:16Z

packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts

+    const invokeScheduleInMinutes = new CfnParameter(stack, 'invokeScheduleInMinutes', {
+      type: 'Number',
+      description: 'The minutes between scheduled invocations.',
+      default: 5,
+    });


If you switch to using the Lazy class (which also uses the same token mechanism), it would be easier to test. Something like this -

const lazyDuration = Duration.minutes(Lazy.number({ produce: () => 5 })); const rate = appscaling.Schedule.rate(lazyDuration) test.equal('rate(<token> minutes)', rate); test.equal('rate(5 minutes)', stack.resolve(rate));

Sure, will give it a try.

nija-at · 2021-02-16T10:36:41Z

packages/@aws-cdk/aws-events/lib/schedule.ts

-  public static rate(duration: Duration): Schedule {
+  public static rate(duration: Duration): Schedule {    
+    if(duration.isUnresolved()){
+      return new LiteralSchedule(`rate(${duration.toHumanString()})`); 


I believe this only supports minutes, hours and days but Duration also supports seconds and milliseconds. It could support more in the future.

Also, I believe the number must not be a fraction

Could we add validations for these, and throw an appropriate error?

I saw there was a testcase in test.cron.ts, test.schedule.ts which was makes sure if the duration was not a whole number, following snippet would throw an error. But this would only happen in case of minutes where integral is sent as true. Looking at duration.toSeconds() === 0 being checked explictly makes me wonder if it been done deliberately.

if (!Number.isInteger(value) && integral) { throw new Error(`'${amount} ${fromUnit}' cannot be converted into a whole number of ${toUnit}.`); }

Also, in order to support the validation we would need to expose a method in duration since unit is not exposed outside.

nija-at · 2021-02-16T10:45:56Z

packages/@aws-cdk/aws-applicationautoscaling/test/test.cron.ts

+    test.done();
+  },
+
+  'rate can be in minutes'(test: Test) {


curious: why do we need this extra test case?

I wanted to make sure we had something around regular minutes.

nija-at · 2021-02-16T10:48:03Z

packages/@aws-cdk/aws-events/test/test.input.ts

+      expect(stack).to(haveResourceLike('AWS::Events::Rule', {
+        Targets: [


We should be asserting the ScheduleExpression property instead of the Targets property, no?

Right, my bad.

nija-at · 2021-02-16T10:51:41Z

packages/@aws-cdk/aws-events/test/test.input.ts

@@ -102,6 +102,49 @@ export = {
      }));
      test.done();
    },
+
+    'can use token from cfnParameter'(test: Test) {


This feels like a duplicate of the test in test.rule.ts. Does this check anything additional that the other test case doesn't?

Let me check and remove if it is duplicate.

nija-at · 2021-02-16T10:52:31Z

packages/@aws-cdk/aws-applicationautoscaling/test/test.scalable-target.ts

@@ -85,6 +85,40 @@ export = {
    test.done();
  },

+  'add scheduled scaling with cfnParameter'(test: Test) {


same as my other comment. Feels like the test in test.cron.ts should provide sufficient cover. Do we need an extra test case?

Sure, let me check and remove if not required.

Pull request has been modified.

nija-at · 2021-02-19T16:07:39Z

I missed this in the first round but toHumanString() is not going to work here. It's because it places '<token>' when it finds an unresolved token.

What should happen for this to work is that the actual token encoded number - https://docs.aws.amazon.com/cdk/latest/guide/tokens.html#tokens_number - must be produced here.

So you will likely have to introduce a new API, say format(), that would do something like this -

const duration = Duration.seconds(5);
const output = duration.format('duration is ${amount} ${unit}');

output <-- "duration is 5 seconds"

With tokens, it would be -

const duration = Duration.seconds(Lazy.number({ produce: () => 5 }));
const output = duration.format('duration is ${amount} ${unit}');

output <-- "duration is -1.8881545897087626e+289 seconds" // this is a number encoded token
stack.resolve(output) <-- "duration is 5 seconds"

sneharathod · 2021-02-20T14:12:15Z

I had tried sending token value as it is like -1.8881545897087626e+289. I had modified toString() in duration.ts and used it instead of humanstring. But it had not worked too with stack.resolve. Will check again.

…

On Fri, Feb 19, 2021 at 5:07 PM Niranjan Jayakar ***@***.***> wrote: I missed this in the first round but toHumanString() is not going to work here. It's because it places '' when it finds an unresolved token. What should happen for this to work is that the actual token encoded number - https://docs.aws.amazon.com/cdk/latest/guide/tokens.html#tokens_number - must be produced here. So you will likely have to introduce a new API, say format(), that would do something like this - const duration = Duration.seconds(5);const output = duration.format('duration is ${amount} ${unit}'); output <-- "duration is 5 seconds" With tokens, it would be - const duration = Duration.seconds(Lazy.number({ produce: () => 5 }));const output = duration.format('duration is ${amount} ${unit}'); output <-- "duration is -1.8881545897087626e+289 seconds" // this is a number encoded tokenstack.resolve(output) <-- "duration is 5 seconds" — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYHIJTV3EMPECAD23PJGLDS72EFZANCNFSM4XVKCSOQ> .

sneharathod · 2021-02-22T20:06:03Z

First time user issues! I am seeing some 80+ merge commits since I pulled from master aws-cdk into my fork repo. Let me know if we could discard this one and create new PR from another fork repository.

nija-at · 2021-02-23T10:30:16Z

@sneharathod - this is quite normal in the CDK repo as we have quite a few commits going in each day.

You can choose one of two things - (a) do nothing; when the PR is approved, our bot will automatically update your branch to track the latest and then merge, or (b) click on the 'Update branch' button at the bottom of the PR that will update your branch to track the latest commit on master.

It is necessary to perform (b) explicitly only if there are merge conflicts with master that need to be resolved manually. You will notified of this in the area that currently says "This branch is out-of-date with the base branch".
Your PR currently has no merge conflicts.

nija-at

Much better and I like the approach. Thanks for the feedback.

Some small suggestions below.

packages/@aws-cdk/aws-applicationautoscaling/lib/schedule.ts

packages/@aws-cdk/aws-events/README.md

nija-at · 2021-03-01T14:12:15Z

packages/@aws-cdk/cfnspec/spec-source/000_CloudFormationResourceSpecification.json

@@ -66658,6 +66658,12 @@
          "Required": false,
          "UpdateType": "Immutable"
        },
+        "EnableExecuteCommand": {


can you take a look at why this is coming in? Seems unrelated to your change.

nija-at · 2021-03-01T14:14:08Z

yarn.lock

@@ -3687,16 +3682,6 @@ dot-prop@^5.1.0:
  dependencies:
    is-obj "^2.0.0"

-dotenv-json@^1.0.0:


You changes should not impact this file either.

packages/@aws-cdk/aws-events/test/test.rule.ts

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

sneharathod · 2021-03-01T14:39:24Z

Sure. I missed them.

…

On Mon, Mar 1, 2021 at 3:22 PM Niranjan Jayakar ***@***.***> wrote: ***@***.**** requested changes on this pull request. Much better and I like the approach. Thanks for the feedback. Some small suggestions below. ------------------------------ In ***@***.***/aws-applicationautoscaling/lib/schedule.ts <#13064 (comment)>: > + if (validDurationUnit.indexOf(duration.unitLabel()) === -1) { + throw new Error('Allowed unit for scheduling is: \'minute\',\'minutes\', \'hour\', \'hours\', \'day\' or \'days\''); Some code readability. ⬇️ Suggested change - if (validDurationUnit.indexOf(duration.unitLabel()) === -1) { - throw new Error('Allowed unit for scheduling is: \'minute\',\'minutes\', \'hour\', \'hours\', \'day\' or \'days\''); + if (!validDurationUnit.includes(duration.unitLabel())) { + throw new Error("Allowed unit for scheduling is: 'minute', 'minutes', 'hour', 'hours', 'day' or 'days'"); ------------------------------ In ***@***.***/aws-events/README.md <#13064 (comment)>: > @@ -83,6 +83,7 @@ onCommitRule.addTarget(new targets.SnsTopic(topic, { ## Scheduling You can configure a Rule to run on a schedule (cron or rate). +Rate can be specified in Minutes/Hours/Days. ⬇️ Suggested change -Rate can be specified in Minutes/Hours/Days. +Rate must be specified in minutes, hours or days. ------------------------------ In ***@***.***/cfnspec/spec-source/000_CloudFormationResourceSpecification.json <#13064 (comment)>: > @@ -66658,6 +66658,12 @@ "Required": false, "UpdateType": "Immutable" }, + "EnableExecuteCommand": { can you take a look at why this is coming in? Seems unrelated to your change. ------------------------------ In yarn.lock <#13064 (comment)>: > @@ -3687,16 +3682,6 @@ dot-prop@^5.1.0: dependencies: is-obj "^2.0.0" -dotenv-json@^1.0.0: You changes should not impact this file either. ------------------------------ In ***@***.***/aws-events/test/test.rule.ts <#13064 (comment)>: > + expect(stack).toMatch({ + 'Resources': { + 'MyScheduledRuleD15B5BA3': { + 'Type': 'AWS::Events::Rule', + 'Properties': { + 'Name': 'rateInMinutes', + 'ScheduleExpression': 'rate(5 minutes)', + 'State': 'ENABLED', + }, + }, + }, + }); When asserting properties of single resources, we use - ⬇️ Suggested change - expect(stack).toMatch({ - 'Resources': { - 'MyScheduledRuleD15B5BA3': { - 'Type': 'AWS::Events::Rule', - 'Properties': { - 'Name': 'rateInMinutes', - 'ScheduleExpression': 'rate(5 minutes)', - 'State': 'ENABLED', - }, - }, - }, - }); + expect(stack).toHaveResourceLike('AWS::Events::Rule', { + 'Name': 'rateInMinutes', + 'ScheduleExpression': 'rate(5 minutes)', + }); ------------------------------ In ***@***.***/core/lib/duration.ts <#13064 (comment)>: > @@ -252,6 +252,28 @@ export class Duration { } return ret; } + + /** Can you add unit tests for the new methods in this file? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13064 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYHIJSPIHMH3HAJVPPIA5DTBOPJHANCNFSM4XVKCSOQ> .

Pull request has been modified.

nija-at

Setting this back to 'request changes' to address remaining comments.

Pull request has been modified.

…g and events to consider token in duration (#9413) feat(events,applicationautoscaling): schedule to check for minutes/hours/days, fixed cr comments, Fix for Issue: #9413 feat(events,applicationautoscaling): schedule fixed cr comments

mergify · 2021-03-09T11:02:15Z

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 · 2021-03-09T14:19:42Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 01e5d5b
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-03-09T14:19:59Z

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

nija-at assigned rix0rrr and nija-at and unassigned rix0rrr Feb 16, 2021

nija-at changed the title ~~fix(core/events/appscaling) : allow schedule in application autoscaling and events to consider token in duration fixes (#9413)~~ feat(events,applicationautoscaling): schedule can be a token Feb 16, 2021

nija-at assigned rix0rrr and unassigned nija-at Feb 16, 2021

nija-at previously requested changes Feb 16, 2021

View reviewed changes

nija-at assigned nija-at and unassigned rix0rrr Feb 19, 2021

nija-at previously requested changes Mar 1, 2021

View reviewed changes

nija-at previously requested changes Mar 3, 2021

View reviewed changes

nija-at approved these changes Mar 9, 2021

View reviewed changes

Merge branch 'master' into master

01e5d5b

mergify bot merged commit b1449a1 into aws:master Mar 9, 2021

This was referenced Mar 12, 2021

build(deps): bump aws-cdk from 1.92.0 to 1.93.0 linz/geostore#393

Merged

build(deps): bump cdk.version from 1.92.0 to 1.93.0 sullis/aws-cdk-java-example#66

Merged

jumic mentioned this pull request Aug 15, 2021

[@aws-cdk/aws-events] Schedule cannot be created from CfnParameter #9413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(events,applicationautoscaling): schedule can be a token #13064

feat(events,applicationautoscaling): schedule can be a token #13064

sneharathod commented Feb 15, 2021

gitpod-io bot commented Feb 15, 2021 •

edited

Loading

mergify bot commented Feb 15, 2021

nija-at left a comment

nija-at Feb 16, 2021

sneharathod Feb 17, 2021

nija-at Feb 16, 2021 •

edited

Loading

sneharathod Feb 17, 2021 •

edited

Loading

nija-at Feb 16, 2021

sneharathod Feb 17, 2021

nija-at Feb 16, 2021

sneharathod Feb 17, 2021

nija-at Feb 16, 2021

sneharathod Feb 17, 2021

nija-at Feb 16, 2021

sneharathod Feb 17, 2021

nija-at commented Feb 19, 2021 •

edited

Loading

sneharathod commented Feb 20, 2021 via email

sneharathod commented Feb 22, 2021

nija-at commented Feb 23, 2021

nija-at left a comment

nija-at Mar 1, 2021

nija-at Mar 1, 2021

sneharathod commented Mar 1, 2021 via email

nija-at left a comment

mergify bot commented Mar 9, 2021

aws-cdk-automation commented Mar 9, 2021

mergify bot commented Mar 9, 2021

		expect(stack).to(haveResourceLike('AWS::Events::Rule', {
		Targets: [

feat(events,applicationautoscaling): schedule can be a token #13064

feat(events,applicationautoscaling): schedule can be a token #13064

Conversation

sneharathod commented Feb 15, 2021

gitpod-io bot commented Feb 15, 2021 • edited Loading

mergify bot commented Feb 15, 2021

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

sneharathod Feb 17, 2021 • edited Loading

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

nija-at commented Feb 19, 2021 • edited Loading

sneharathod commented Feb 20, 2021 via email

sneharathod commented Feb 22, 2021

nija-at commented Feb 23, 2021

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sneharathod commented Mar 1, 2021 via email

nija-at left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 9, 2021

aws-cdk-automation commented Mar 9, 2021

AWS CodeBuild CI Report

mergify bot commented Mar 9, 2021

gitpod-io bot commented Feb 15, 2021 •

edited

Loading

nija-at Feb 16, 2021 •

edited

Loading

sneharathod Feb 17, 2021 •

edited

Loading

nija-at commented Feb 19, 2021 •

edited

Loading