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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Oct 26, 2023
Merged

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

merged 6 commits into from
Oct 26, 2023

Conversation

markanderstam
Copy link
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
aws-cdk-automation
aws-cdk-automation previously requested changes Oct 24, 2023
View reviewed changes
Copy link
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
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
scanlonp
scanlonp requested changes Oct 25, 2023
View reviewed changes
Copy link
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.

...ges/aws-cdk-lib/aws-ecs-patterns/lib/base/application-multiple-target-groups-service-base.ts Outdated Show resolved Hide resolved
markanderstam and others added 2 commits October 26, 2023 11:22
@markanderstam @scanlonp
fix: typo
5138ebc 
Co-authored-by: Parker Scanlon <69879391+scanlonp@users.noreply.github.com>
@markanderstam
fix: false positive in idleTimeout integ test for multi alb service
3a4f753
@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.

scanlonp
scanlonp previously requested changes Oct 26, 2023
View reviewed changes
Copy link
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
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..)

scanlonp
scanlonp approved these changes Oct 26, 2023
View reviewed changes
Copy link
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
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
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
8 checks passed
@mergify
Copy link
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).

@github-actions github-actions bot mentioned this pull request Nov 1, 2023
Closed
mrgrain pushed a commit that referenced this pull request Nov 1, 2023
@markanderstam @mrgrain
fix: duration doesn't get accurately compared in multi alb service ba…
bb6191b 
…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
Reviewers

@scanlonp scanlonp scanlonp approved these changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees

@scanlonp scanlonp

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markanderstam @aws-cdk-automation @scanlonp