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.

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

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

Merged
merged 2 commits into from
Mar 9, 2021
Merged

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

merged 2 commits into from
Mar 9, 2021

Conversation

sneharathod
Copy link
Contributor

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
Copy link

gitpod-io bot commented Feb 15, 2021

@mergify
Copy link
Contributor

mergify bot commented Feb 15, 2021

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

@nija-at nija-at assigned rix0rrr and nija-at and unassigned rix0rrr Feb 16, 2021
@nija-at 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 nija-at assigned rix0rrr and unassigned nija-at Feb 16, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 32 to 36
const invokeScheduleInMinutes = new CfnParameter(stack, 'invokeScheduleInMinutes', {
type: 'Number',
description: 'The minutes between scheduled invocations.',
default: 5,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will give it a try.

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

@nija-at nija-at Feb 16, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@sneharathod sneharathod Feb 17, 2021

Choose a reason for hiding this comment

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

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.

test.done();
},

'rate can be in minutes'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

curious: why do we need this extra test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 125 to 126
expect(stack).to(haveResourceLike('AWS::Events::Rule', {
Targets: [
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, my bad.

@@ -102,6 +102,49 @@ export = {
}));
test.done();
},

'can use token from cfnParameter'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check and remove if it is duplicate.

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

'add scheduled scaling with cfnParameter'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me check and remove if not required.

@mergify mergify bot dismissed nija-at’s stale review February 17, 2021 10:38

Pull request has been modified.

@nija-at
Copy link
Contributor

nija-at commented Feb 19, 2021

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"

@nija-at nija-at assigned nija-at and unassigned rix0rrr Feb 19, 2021
@sneharathod
Copy link
Contributor Author

sneharathod commented Feb 20, 2021 via email

@sneharathod
Copy link
Contributor Author

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
Copy link
Contributor

nija-at commented Feb 23, 2021

@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
nija-at previously requested changes Mar 1, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

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

Some small suggestions below.

packages/@aws-cdk/aws-events/README.md Outdated Show resolved Hide resolved
@@ -66658,6 +66658,12 @@
"Required": false,
"UpdateType": "Immutable"
},
"EnableExecuteCommand": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

dotenv-json@^1.0.0:
Copy link
Contributor

Choose a reason for hiding this comment

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

You changes should not impact this file either.

packages/@aws-cdk/aws-events/test/test.rule.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/duration.ts Show resolved Hide resolved
@sneharathod
Copy link
Contributor Author

sneharathod commented Mar 1, 2021 via email

@mergify mergify bot dismissed nija-at’s stale review March 2, 2021 13:24

Pull request has been modified.

nija-at
nija-at previously requested changes Mar 3, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

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

@mergify mergify bot dismissed nija-at’s stale review March 5, 2021 19:45

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
Copy link
Contributor

mergify bot commented Mar 9, 2021

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
Copy link
Collaborator

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
Copy link
Contributor

mergify bot commented Mar 9, 2021

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

This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants