Skip to content

fix: duration doesn't get accurately compared in multi alb service base#27664

Merged
mergify[bot] merged 6 commits intoaws:mainfrom
markanderstam:patch-1
Oct 26, 2023
Merged

fix: duration doesn't get accurately compared in multi alb service base#27664
mergify[bot] merged 6 commits intoaws:mainfrom
markanderstam:patch-1

Conversation

@markanderstam
Copy link
Copy Markdown
Contributor

Duration wasn't getting compared correctly and prevented some values, eg between 5 and 9 or 500 and 999 (perhaps others as well), from getting used for ApplicationMultipleTargetGroupsService.idleTimeout.

Relates to #21584, that fixes this issue for ApplicationLoadBalancedService (however the problem description was not fully correct in regards to what range of values were getting denied).


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

@aws-cdk-automation aws-cdk-automation requested a review from a team October 24, 2023 16:03
@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Oct 24, 2023
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@markanderstam
Copy link
Copy Markdown
Contributor Author

Exemption Request

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Oct 25, 2023
@scanlonp scanlonp self-assigned this Oct 25, 2023
Copy link
Copy Markdown
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

Looks good! We already have an idle timeout integ test (here) so changing the timeout values there to match your unit test would be prefered, but not strictly necessary.

markanderstam and others added 2 commits October 26, 2023 11:22
Co-authored-by: Parker Scanlon <69879391+scanlonp@users.noreply.github.com>
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 26, 2023 11:42

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Copy link
Copy Markdown
Contributor

@scanlonp scanlonp 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 the integ test update!
Last thing, do not think we need to update yarn.lock. Mind removing the change from the PR unless it is necessary?

@mergify mergify bot dismissed scanlonp’s stale review October 26, 2023 15:06

Pull request has been modified.

@markanderstam
Copy link
Copy Markdown
Contributor Author

Thanks for the integ test update! Last thing, do not think we need to update yarn.lock. Mind removing the change from the PR unless it is necessary?

Sorry about that, I will revert that change.

(Also, didn't realize that "Remove file" from the file viewer actually deleted the file and not the changes 😄 .. fixing..)

Copy link
Copy Markdown
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

Wonderful. Thanks!

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 26, 2023

Thank you for contributing! Your pull request will be updated from main 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
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 29c2ed1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 7e426c8 into aws:main Oct 26, 2023
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 26, 2023

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

mrgrain pushed a commit that referenced this pull request Nov 1, 2023
…se (#27664)

Duration wasn't getting compared correctly and prevented some values, eg between 5 and 9 or 500 and 999 (perhaps others as well), from getting used for `ApplicationMultipleTargetGroupsService.idleTimeout`.

Relates to #21584, that fixes this issue for ApplicationLoadBalancedService (however the problem description was not fully correct in regards to what range of values were getting denied).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants