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(cdk): Added SecondsBeforeTimeout, TimeOutAction #27183 #27193

Closed
wants to merge 0 commits into from
Closed

feat(cdk): Added SecondsBeforeTimeout, TimeOutAction #27183 #27193

wants to merge 0 commits into from

Conversation

edxlang
Copy link

@edxlang edxlang commented Sep 19, 2023

Added secondsBeforeTimeout
Added timeOutAction

Followed coding conventions,
Enacted checks for both seconds before the timeout and time-out action.

Having trouble testing locally
Closes #27183.


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

@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Sep 19, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team September 19, 2023 12:54
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.

@edxlang edxlang changed the title PR: SecondsBeforeTimeout, TimeOutAction #27183 Feat: Added SecondsBeforeTimeout, TimeOutAction #27183 Sep 19, 2023
@edxlang edxlang marked this pull request as draft September 19, 2023 13:04
@edxlang edxlang changed the title Feat: Added SecondsBeforeTimeout, TimeOutAction #27183 feat(cdk): Added SecondsBeforeTimeout, TimeOutAction #27183 Sep 20, 2023
@edxlang
Copy link
Author

edxlang commented Sep 25, 2023

Clarification Request - I cannot change the README file and I'm having trouble navigating to find the correct integration test for this ---- thank you

@aws-cdk-automation aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Sep 25, 2023
@edxlang edxlang marked this pull request as ready for review September 25, 2023 22:09
@edxlang
Copy link
Author

edxlang commented Sep 26, 2023

Clarification Request - I'm having trouble navigating to find the correct integration test for this and help with running the tests for a snapshot

@edxlang
Copy link
Author

edxlang commented Sep 27, 2023

Exemption Request: ReadMe and Integration Tests

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Sep 27, 2023
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.

Thank you for the contribution!

I believe that this is the integ test you are looking for: https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-rds/test/integ.serverless-cluster.ts (or possibly one of the others in that folder).
All of our integ tests for modules in aws-cdk-lib live in @aws-cdk-testing/framework-integ.

Could you provide more detail on not being able to edit the readme? This file: https://github.com/aws/aws-cdk/tree/main/packages/aws-cdk-lib/aws-rds, correct?

Once you have found these files, and you have a good reason for not needing to change them, ping me again!
Otherwise, see my comments and reach out if you need any support!

packages/aws-cdk-lib/aws-rds/lib/serverless-cluster.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-rds/lib/serverless-cluster.ts Outdated Show resolved Hide resolved
@scanlonp scanlonp self-assigned this Oct 2, 2023
@edxlang
Copy link
Author

edxlang commented Oct 3, 2023

So, I'm still having some trouble running integration tests, but I have updated the code and readme. Still working on integ testing

@edxlang edxlang requested a review from scanlonp October 3, 2023 23:57
@scanlonp
Copy link
Contributor

scanlonp commented Oct 4, 2023

@0xedl hey there, I will not be able to give a meaningful review until the build is passing. You can open the build logs and search for !!!!!!! to find the error (try starting from the bottom when searching, it will be the last error). Your current error looks like timeout is not a property on the Cfn level resource in the return statement (line 489 in serverless-cluster.ts) since we changed it for the public API. Keep iterating until the build is passing, and then I will check back in!

@edxlang
Copy link
Author

edxlang commented Oct 5, 2023

Hey! I understand. The issue is running the tests in general - when I try to the integ tests, I get an integ not found error. I think I'm getting package installation errors. Ive tried running the default integ test and same issue. Could this be caused by the build issue?

@scanlonp
Copy link
Contributor

scanlonp commented Oct 5, 2023

If you believe its a build error, try wiping all built files and re-building.
In the root of aws-cdk something like:

> git clean -fqdx .
> NODE_OPTIONS=--max-old-space-size=8192 npx lerna run build
if needed
> NODE_OPTIONS=--max-old-space-size=8192 npx lerna run build --scope=@aws-cdk/integ-tests-alpha
and/or
> NODE_OPTIONS=--max-old-space-size=8192 npx lerna run build --scope=@aws-cdk-testing/framework-integ

@edxlang
Copy link
Author

edxlang commented Oct 6, 2023

Apparently, I messed something up in my local workspace. and I am missing some dependencies which doesn't seem to be working at all. Fortunately, GH workspace is workgin, but integ testing may take a minute! I will update the PR when it is ready. thank you.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Oct 6, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 7, 2023 06:37

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

@mergify mergify bot dismissed scanlonp’s stale review October 7, 2023 15:41

Pull request has been modified.

@edxlang
Copy link
Author

edxlang commented Oct 7, 2023

@scanlonp Hey, I'm having a weird issue with the final build and third party licenses being outdated....I've completed everything else and have been trying to fix this for a while now by rebuilding, merging from main, etc.

In package package.json

  • [@aws-cdk/node-bundle => outdated-attributions] THIRD_PARTY_LICENSES is outdated (fixable)
  • at Module._compile (node:internal/modules/cjs/loader:1256:14)
    
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:119:18)

I've tried rebasing, rebuilding from the main repo and nothing is working atm.

@scanlonp
Copy link
Contributor

scanlonp commented Oct 7, 2023

Hey @0xedl, it looks like your PR has a number of changes to package files, including deleting lazify & a number of related files. I also noticed that your pull is from the main branch of your fork. We do have a requirement that pulls must come from non-main branches to enable our automation to work correctly (read more here CONTRIBUTING.md).

I would recommend you start a new branch off a fresh copy of our main, which may solve both issues!

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@edxlang
Copy link
Author

edxlang commented Oct 11, 2023

still workoing on building it correctly...... the features are implemented correctly,but just the build issues.

@edxlang edxlang closed this Oct 11, 2023
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.

@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to a test file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@scanlonp scanlonp removed their assignment Jan 23, 2024
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 effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr/needs-cli-test-run This PR needs CLI tests run against it. pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run 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.

aws-rds : Allow full support for scaling configuration options
4 participants