fix(awslint): include types from submodules during linting (WIP) #26942

mikewrighton · 2023-08-30T15:55:36Z

Addresses #26839

The initial change in this draft modifies some of the linters within the awslint tool so that they include types from submodules, when reading from the JSII assembly. The results in a large number of lint errors since it causes L1 constructs to be included incorrectly - this is happening because 1) L1 constructs are submodules in the assembly, and 2) the CoreTypes.isCfnResource() function no longer works as it relies on the legacy core module which no longer exists.

Follow-up work:

Fix CoreTypes and any other code within awslint which references @aws-cdk/core
Fix this broken code (which becomes active after Initial external release #1) which relies on a docstring pattern to generate the CFN resource name. Using a tag like @cloudformationResource (need re-adding) is probably a better approach.
Any other fixes needed on the linter code paths which are now active.

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

aws-cdk-automation

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.

mikewrighton · 2023-08-30T16:14:03Z

packages/awslint/lib/rules/docs.ts

@@ -53,7 +53,9 @@ docsLinter.add({
    if (e.ctx.kind !== 'interface-property') { return; }
    if (!e.ctx.containingType.isDataType()) { return; }
    // this rule does not apply to L1 constructs
-    if (CoreTypes.isCfnType(e.ctx.containingType)) { return; }
+    if (CoreTypes.isCfnType(e.ctx.containingType)) {
+      return;


Changed for debugging purposes - can revert.

Its deprecation is causing problems for `cdk-nag`, which has a use case for unsuppressible warnings. Undeprecate for now until we come up with a better all-round solution. Relates to #26914. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

I felt it best to keep most of the details in the linked documentation, but I believe this provides a good overview. Closes #21103. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…ld counts (#26949) `HealthyThresholdCount` and `UnhealthyThresholdCount` do not need to be the same. In fact, the [docs](https://docs.aws.amazon.com/elasticloadbalancing/latest/network/target-group-health-checks.html#health-check-settings) have their default values as 5 and 2. Closes #26941. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…s-cdk/aws-lambda-python-alpha/test/lambda-handler-poetry (#26956) Bumps [certifi](https://github.com/certifi/python-certifi) from 2023.5.7 to 2023.7.22. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/certifi/python-certifi/commit/8fb96ed81f71e7097ed11bc4d9b19afd7ea5c909"><code>8fb96ed</code></a> 2023.07.22</li> <li><a href="https://github.com/certifi/python-certifi/commit/afe77220e0eaa722593fc5d294213ff5275d1b40"><code>afe7722</code></a> Bump actions/setup-python from 4.6.1 to 4.7.0 (<a href="https://github.com/certifi/python-certifi/issues/230">#230</a>)</li> <li><a href="https://github.com/certifi/python-certifi/commit/2038739ad56abec7aaddfa90ad2ce6b3ed7f5c7b"><code>2038739</code></a> Bump dessant/lock-threads from 3.0.0 to 4.0.1 (<a href="https://github.com/certifi/python-certifi/issues/229">#229</a>)</li> <li><a href="https://github.com/certifi/python-certifi/commit/44df761f4c09d19f32b3cc09208a739043a5e25b"><code>44df761</code></a> Hash pin Actions and enable dependabot (<a href="https://github.com/certifi/python-certifi/issues/228">#228</a>)</li> <li>See full diff in <a href="https://github.com/certifi/python-certifi/compare/2023.05.07...2023.07.22">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=certifi&package-manager=pip&previous-version=2023.5.7&new-version=2023.7.22)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/aws/aws-cdk/network/alerts). </details>

…26917) Currently, the region parameter in `AwsCustomResource` only controls where the action is performed. If a role needs to be assumed, the `assumeRole` call is made from the region the stack is deployed into. This presents a problem if the stack is deployed into an opt-in region, and the role being assumed lives in a separate stack in an account without the opt-in region enabled. This change makes the `assumeRole` call and the sdk call performed in the same region. Therefore, to solve the above problem, pass any region that is enabled for the account that owns the role to be assumed. Closes #26562. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action

Reverts #26563

…#26962) Closes #26952. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This adds the ability to use the latest two NodeJS runtimes in your Synthetics construct. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…nnot be set at once (#26971) This PR adds comments that both defaultInterval and start for dashboard cannot be set at once. The following is a validation code for them. https://github.com/go-to-k/aws-cdk/blob/05d1c524e356f0e3cbd4799c6f7d233a44301954/packages/aws-cdk-lib/aws-cloudwatch/lib/dashboard.ts#L135-L137 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>

AWS Service Spec packages to latest versions.

I found some grammatical errors while debugging an issue. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

) When a DeliveryStream is created without `sourceStream` or `encryptionKey`, an extra role is being created that is unused. This PR removes creation of that role. I also learned that the role created for `encryptionKey` is used "indirectly" for a grant put on the KMS key...interesting. Closes #26927. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The upcoming v6 of the [VSCode jest plugin](https://github.com/jest-community/vscode-jest#virtualfolders) allows us to configure our monorepo packages as virtual folders. This makes the built-in Test Explorer work out of the box. <img width="466" alt="image" src="https://github.com/aws/aws-cdk/assets/379814/b98b3736-4713-49d7-bec6-816f14816e73"> Using the workspace is entirely optional. It can also serve as an example configurations. But it does give a space to add recommended/opinionated VSCode settings without compromising contributor's ability to change `.vscode/settings.json`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…16 (#26980) Updates the `CrossAccountZoneDelegationRecord` construct to use sdk v3 / node 18. This is identical to changes in #26212, except for hardcoding a region into the `assumeRole` sdk call. This may not be the ideal solution, but should not break specific configurations. That specific configuration, as #26593 pointed out, was that the original update was a breaking change if the construct was deployed into an opt-in region, and the parent zone did not have that opt-in region enabled. This PR removes the semi-hidden `@aws-cdk/aws-route53:useRegionalStsEndpoint` feature flag, as it was based on a confusion on why things used to work. We now pick the correct endpoint manually. Closes #26976. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Fixes to remove an incorrectly generated warning when using Node JS runtime <= 16 with `NodejsFunction` Closes #26966 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…#26951) Checks off some items from #26936 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

) We use a custom resource to set the log retention for log groups created by the Lambda service. This custom resource handler code has a built-in retry mechanism to avoid throttling when executing many LogRetention CRs. Users can customize the number of possible retries, potentially retrying for a long time. This can cause the situation that further retries should be attempted, but the Lambda Function timeout is exceeded. The change sets the lambda execution timeout to its maximum value to allow for up to 15 minutes of retries. If the retry budget is exhausted, the handler will throw an error and exit early. Closes #24485 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

`apiVersion` no longer is recognized by sdk v3. source: aws/aws-sdk-js-v3#1869 We use it in two custom resources. I confirmed that each `apiVersion` represented the latest version of sdk v2 and was almost certainly hardcoded because it was the recommended practice: https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/locking-api-versions.html Both integ tests succeed deployment. I can confirm that `aws-route53/delete-existing-record-set-handler` works as intended but the integ test for `pipelines/approve-lambda` could never have worked and needs to be rewritten. That should happen in a different PR. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…t failure (#27000) The payload response of a Lambda used to be a `string`, and could occasionally be `""`, which we detected and special-case parsed to an empty object. The Payload should never be empty, and will only be that under exceptional circumstances which we haven't been able to pin down yet, but we shouldn't fail in any case. In SDKv3, the payload response of a Lambda changed to type `Uint8Array`, but a `Uint8Array(0)` doesn't check as *falsey*, so we'd decode it to `""` and then the `JSON.parse()` of that would fail. First decode, then check the string for emptyness. Fixes #26429.

…ry (#27092) #27009 introduced support for `Uint8Array` if users want to send data with the same types as those declared in the SDK v3 API. But existing tests, that use strings, will break. Apply type coercion when necessary. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…Endpoints (#27093) This also tests the usage of AwsCustomResource to describe security groups. Manually tested the unhappy path: If not SG can be found, the stack deployment fails as expected. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Changing the table structure to include an initial `TableBase` abstract class, allowing different tables of different data sources to be created from. Initially there are two, `S3Table` and `ExternalTable`. - `S3Table`: The current table structure that has been used throughout the previous versions of the CDK - `ExternalTable`: The new glue table that will be used to store metadata about external data sources. This subclass will contain an `externalDataLocation` property to explicitly specify the `Location` property of the underlying `CfnTable` L1 construct - `Table`: This is now `@deprecated` to shift the usage towards `S3Table` Closes #24741. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

AWS Service Spec packages to latest versions.

…27096) The source code in `custom-resource-handlers` was not included in the `npm pack` and thus unavailable to customers. This PR includes it. Tested by running `npm pack` and confirming its existence and also testing it the resulting package on a local cdk app. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Upgrades remaining runtime versions from `NODEJS_16_X` (which enters EOL on 2023-09-11 and will enter Lambda's Deprecation Phase 1 on Mar 11 2024) to NODEJS_18_X. Also updates remaining unit & integration tests to use NODEJS_LATEST or NODEJS_18_X. Most tests will reference the lambda runtime version via a new centralized configuration. Updated a number of test handlers to use SDKv3 and cleaned-up some duplicated, unused handler files. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Adding an integ test for this custom resource with our new rule that each custom resource needs at least one integ test. There is nothing to assert here, since the custom resource calls an AWS api to return a string. Asserting it with the same api call seems redundant. Instead, I'm going with supplying a cfnOutput to the console, which shows the string is zookeeper related. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

) Experimental custom resource handlers should be airlifted to their experimental modules only. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…rce (#27106) Adding an integ test for this custom resource with our new rule that each custom resource needs at least one integ test. This integ test is asserting that one scram secret per user is being registered with the cluster when SASL/SCRAM authentication is used. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…of aws-stepfunctions (#27110) `definition` in Construct Props of `sfn.StateMachine` is already deprecated and `definitionBody` is recommended. README and docs still use `definition` in examples, and it's misleading for users. I replaced `definition` with `definitionBody` and `sfn.DefinitionBody.fromChainable(...)` in them. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…itory (#27115) It is not clear whether we can use image digest on `ContainerImage.fromEcrRepository`. Change the property name from `tag` to `tagOrDigest` to clarify the ability to use image digest. Also add document about the parameter format. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Adding an integ test for this custom resource with our new rule that each custom resource needs at least one integ test. Adding this test for the custom resource to test out AWS Amplify app. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Adds integration test for using cross-region reference custom resource handlers. Currently cross region multi-stack tests in `IntegTest` are not super well supported so assertions aren't a great option but a successful stack deploy and delete does indicate success of the CRs currently. Closes #27069 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

For technical reasons, the way we were importing `minimatch` works in the current version of TypeScript, but is actually illegal according to the ESM spec. This poses problems when running `aws-cdk-lib` through `esbuild`. Update to a different import style that is correct in both. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

I found a legit zip [here](https://github.com/aws/aws-cdk/blob/d8acc8aa07867be1b1b3cad05b67dab2d7bc3252/packages/%40aws-cdk/aws-codepipeline-actions/test/assets/nodejs.zip). Closes #27117. <img width="1211" alt="Screenshot 2023-09-12 at 6 46 22 PM" src="https://github.com/aws/aws-cdk/assets/3310356/1de56fcf-1f99-4095-b2d9-1668bd443a9f"> ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

With Triggers a user can invoke a Lambda Function during every stack deployment. When we migrated the handler code to SDKv3, we overlooked that the return payload from the invoked lambda function is now a `Uint8Array`, and not a string anymore. In the error case, we attempt to `JSON.parse()` the error payload which then resulted in a list of bytes being returned to the user. Updated the handler test case to use correct mocks, including returning a payload that is a Uint8Array. Closes #27113 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@peterwoodworth

…runtime (#27109) This PR addresses an issue encountered when setting up an AWS CDK pipeline where unresolved token values for the `repoString` parameter led to a runtime error. The updated docstring now explicitly mentions that the `repoString` must be resolvable at runtime and that unresolved tokens are intended to be a runtime error Closes #27100. Thanks to @peterwoodworth for the quick assessment and the proposed solution to the problem. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@iliapolo

…7122) This PR fixes an issue where our assertions handler will fail when logging the response of an API call if the response body is a Blob. Specifically, the following error is thrown: ``` Converting circular structure to JSON --> starting at object with constructor 'TLSSocket' | property 'parser' -> object with constructor 'HTTPParser' --- property 'socket' closes the circle (RequestId: 50c1b6cd-47d2-494f-baf1-d22646cd4e5f) ``` The fix for this issue was to call the `transformToString` method on the response body. This is mentioned in aws-sdk-js-v3 [`UPGRADE.md`](https://github.com/aws/aws-sdk-js-v3/blob/main/UPGRADING.md?plain=1#L573-L576) as the solution for `Lambda::Invoke`. Its unclear why `S3::GetObject` isn't mentioned as well, but it works for that too. However, instead of explicitly extracting the `Body` property from a response, we now recursively traverse the response and detect any such blob types that should be converted to strings - this should protect us against any other APIs that may exhibit this behavior. > `transformToString` is implemented as part of the [`SdkStreamMixin` interface](https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-smithy-types/Interface/SdkStreamMixin/). Included in this PR is a fix for `aws-s3-deployment/test/integ.bucket-deployment-substitution.ts` which was previously failing with the error shown above. Co-authored by: @iliapolo Co-authored by: @vinayak-kukreja Co-authored by: @scanlonp Closes #27114 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…cted, skips recursive types (#27112) AWS SDK v3 strictly validates numeric types, as opposed to v2, which allowed, for example, "123" to be passed where an integer was expected. This script adds all attributes of a numeric type ("byte", "short", "integer", "long", "bigInteger", "float", "double" and "bigDecimal") to a new type coercion map. It also compacts the map by trimming each part of the path to its unique prefix among all the paths in its operation. We also change the data structure from a list of paths (forming a tree) to a graph/state machine. This is necessary in order to support self-recursive types, which can loop arbitrarily and therefore cannot be represented in fixed length prefix lists. See aws/aws-sdk-js-v3#5198 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…mEcrRepository" (#27143) Reverts #27115 Renaming `tag` prop to `tagOrDigest` of the `ContainerImage.fromEcrRepository` function introduces a breaking change, not for typescript users, but for other languages that require specifying the property name when calling the function.

aws-cdk-automation · 2023-09-14T22:05:31Z

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/26942/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

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 · 2023-09-14T22:06:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 35dc903
Result: FAILED
Build Logs (available for 30 days)

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

Include types deom submodules in awslint.

dd52f5f

github-actions bot added the p2 label Aug 30, 2023

aws-cdk-automation requested a review from a team August 30, 2023 15:55

mikewrighton changed the title ~~fix(awslint): include types from submodules during linting~~ fix(awslint): include types from submodules during linting (WIP) Aug 30, 2023

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 30, 2023

aws-cdk-automation requested changes Aug 30, 2023

View reviewed changes

mikewrighton commented Aug 30, 2023

View reviewed changes

rix0rrr and others added 23 commits September 14, 2023 18:00

chore: update Contributors File (#26961)

b2bdbb7

Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action

revert: "feat(dynamodb): global table L2" (#26960)

64d72fd

Reverts #26563

chore(integ-tests-alpha): add redshift serverless to sdk api metadata (…

e1d9c95

…#26962) Closes #26952. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(synthetics-alpha): add latest two NodeJS runtimes (#26967)

ba786ec

This adds the ability to use the latest two NodeJS runtimes in your Synthetics construct. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

chore(release): 2.94.0

d39189c

chore: update changelog

c169d8a

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>

feat: update AWS Service Spec (#26999)

c127d33

AWS Service Spec packages to latest versions.

docs(ssm): fix grammatical errors for StringParameter (#27004)

159ed7e

I found some grammatical errors while debugging an issue. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

chore(region-info): Add missing information for all supported regions (…

7f363c8

…#26951) Checks off some items from #26936 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

dependabot bot and others added 25 commits September 14, 2023 18:00

feat: update AWS Service Spec (#27095)

3385e92

AWS Service Spec packages to latest versions.

chore: github actions do not accept node18 (#27104)

608aec9

chore(release): 2.96.0

3bab59f

awslint fixes.

e040ab5

Add awslint exclusions after linter fixes.

35dc903

mikewrighton had a problem deploying to test-pipeline September 14, 2023 22:02 — with GitHub Actions Failure

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Sep 14, 2023

mikewrighton closed this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(awslint): include types from submodules during linting (WIP) #26942

fix(awslint): include types from submodules during linting (WIP) #26942

mikewrighton commented Aug 30, 2023 •

edited

Loading

aws-cdk-automation left a comment

mikewrighton Aug 30, 2023

aws-cdk-automation commented Sep 14, 2023

aws-cdk-automation commented Sep 14, 2023

fix(awslint): include types from submodules during linting (WIP) #26942

fix(awslint): include types from submodules during linting (WIP) #26942

Conversation

mikewrighton commented Aug 30, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

mikewrighton Aug 30, 2023

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 14, 2023

aws-cdk-automation commented Sep 14, 2023

AWS CodeBuild CI Report

mikewrighton commented Aug 30, 2023 •

edited

Loading