chore(integ-tests): add waiterProvider to IApiCall#27844
chore(integ-tests): add waiterProvider to IApiCall#27844mergify[bot] merged 14 commits intoaws:mainfrom
Conversation
lpizzinidev
left a comment
There was a problem hiding this comment.
Thanks, just a minor documentation adjustment on the current implementation.
Also:
- I think that an integration test should be added to verify that the policy is working as expected.
- Any reason why
waiterProvidershouldn't be an abstract property inApiCallBaseand used inHttpApiCallas well after this change?
| * access the AssertionsProvider for the waiter state machine. | ||
| * This can be used to add additional IAM policies | ||
| * the the provider role policy |
There was a problem hiding this comment.
| * access the AssertionsProvider for the waiter state machine. | |
| * This can be used to add additional IAM policies | |
| * the the provider role policy | |
| * Access the AssertionsProvider for the waiter state machine. | |
| * This can be used to add additional IAM policies | |
| * to the provider role policy. |
Can you please adjust the documentation for provider as well?
Yes, I will add the test.
I think there is probably no case to use |
This Also, is unit testing alone not enough? If it is just a policy check, it seems to me that a unit test alone would suffice. |
|
@go-to-k We've integration tests for the Also, it should be documented here. |
Thanks for letting me know! I missed it, I'll just put it here: integ-tests-alpha/test/assertions/providers/lambda-handler/integ.waiter-provider.ts.
I can't find this test in
Yes, I will take it. |
|
In the meantime, I've created an integ test, what do you think? (I'll change the README later.) |
lpizzinidev
left a comment
There was a problem hiding this comment.
The integration test looks good (just a minor adjustment is needed in my opinion) 👍
I'll review the README documentation when ready.
I can't find this test in providers/integ.assertions.ts, which file is it?
provider.addToRolePolicy is already used in many integration tests across the project.
| integ.assertions.awsApiCall('S3', 'listObjectsV2', { | ||
| Bucket: bucket.bucketName, | ||
| MaxKeys: 1, | ||
| }).expect(ExpectedResult.objectLike({ | ||
| KeyCount: 0, | ||
| })).waitForAssertions({ | ||
| interval: Duration.seconds(10), | ||
| totalTimeout: Duration.minutes(3), | ||
| }).waiterProvider?.addToRolePolicy({ | ||
| Effect: 'Allow', | ||
| Action: ['s3:GetObject', 's3:ListBucket'], | ||
| Resource: [ | ||
| `${bucket.bucketArn}`, | ||
| `${bucket.bucketArn}/*`, | ||
| ], | ||
| }); No newline at end of file |
There was a problem hiding this comment.
| integ.assertions.awsApiCall('S3', 'listObjectsV2', { | |
| Bucket: bucket.bucketName, | |
| MaxKeys: 1, | |
| }).expect(ExpectedResult.objectLike({ | |
| KeyCount: 0, | |
| })).waitForAssertions({ | |
| interval: Duration.seconds(10), | |
| totalTimeout: Duration.minutes(3), | |
| }).waiterProvider?.addToRolePolicy({ | |
| Effect: 'Allow', | |
| Action: ['s3:GetObject', 's3:ListBucket'], | |
| Resource: [ | |
| `${bucket.bucketArn}`, | |
| `${bucket.bucketArn}/*`, | |
| ], | |
| }); | |
| const listObjectsCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', { | |
| Bucket: bucket.bucketName, | |
| MaxKeys: 1, | |
| }).expect(ExpectedResult.objectLike({ | |
| KeyCount: 0, | |
| })).waitForAssertions({ | |
| interval: Duration.seconds(10), | |
| totalTimeout: Duration.minutes(3), | |
| }); | |
| listObjectsCall.waiterProvider?.addToRolePolicy({ | |
| Effect: 'Allow', | |
| Action: ['s3:GetObject', 's3:ListBucket'], | |
| Resource: [ | |
| `${bucket.bucketArn}`, | |
| `${bucket.bucketArn}/*`, | |
| ], | |
| }); |
To keep it cleaner.
|
Thanks for your review. I changed them, and added the case to README. Please check them. |
| backoffRate: 3, | ||
| }); | ||
|
|
||
| apiCall?.waiterProvider.addToRolePolicy({ |
There was a problem hiding this comment.
| apiCall?.waiterProvider.addToRolePolicy({ | |
| apiCall.waiterProvider?.addToRolePolicy({ |
| In cases where the `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed | ||
| by the `waiterProvider`. | ||
|
|
||
| Same as `provider`, by default, the `AwsApiCall` construct will automatically add the correct IAM policies |
There was a problem hiding this comment.
| Same as `provider`, by default, the `AwsApiCall` construct will automatically add the correct IAM policies | |
| By default, the `AwsApiCall` construct will automatically add the correct IAM policies |
| In cases where the `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed | ||
| by the `waiterProvider`. |
There was a problem hiding this comment.
| In cases where the `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed | |
| by the `waiterProvider`. | |
| When `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed | |
| by the `waiterProvider` assertion provider. |
|
Thanks! I changed. |
|
Thank you for the contribution @go-to-k. The changes look good to me but, merge from |
| * Resource: ['*'], | ||
| * }); | ||
| */ | ||
| public waiterProvider?: AssertionsProvider; |
There was a problem hiding this comment.
Looks like the build is failing here. Can't remove this docstring entirely since its a public API. Why was it removed in the first place?
There was a problem hiding this comment.
I see, I added it anyway.
I've moved the same explanation to IApiCall, and at few lines up public readonly provider: AssertionsProvider; had no docstring, so I removed this too. Why is it good that there is no docstring there (provider)?
export class AwsApiCall extends ApiCallBase {
public readonly provider: AssertionsProvider;I looked aws.json(packages/@aws-cdk/integ-tests-alpha/awslint.json), but could not find the property.
There was a problem hiding this comment.
I understood, that's because it's readonly.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Reverts #27844. This change broke the pipeline, pacmak fails with: ``` #STDOUT> /tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon/CDK/IntegTests/Alpha/ApiCallBase.cs(231,77): error CS0115: 'ApiCallBase._Proxy.WaiterProvider': no suitable method found to override [/tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon.CDK.IntegTests.Alpha.csproj] ```
This PR changes to add the `waiterProvider` property to an `IApiCall` for `awsApiCall` in integ-tests-alpha.
By default `awsApiCall` in integ tests, the AwsApiCall construct will automatically add the correct IAM policies to allow the Lambda function to make the API call. It does this based on the service and api that is provided. In the following example the service is SQS and the api is receiveMessage so it will create a policy with Action: 'sqs:ReceiveMessage'.
```ts
const integ = new IntegTest(app, 'Integ', {
testCases: [stack],
});
integ.assertions.awsApiCall('SQS', 'receiveMessage', {
QueueUrl: 'url',
});
```
There are some cases where the permissions do not exactly match the service/api call, for example the S3 listObjectsV2 api. In these cases it is possible to add the correct policy by accessing the `provider` object.
```ts
const apiCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', {
Bucket: 'mybucket',
});
apiCall.provider.addToRolePolicy({
Effect: 'Allow',
Action: ['s3:GetObject', 's3:ListBucket'],
Resource: ['*'],
});
```
On the other hand, there is the case to use `waitForAssertions` when using `awsApiCall` in integ tests. This causes `apiCall` to have a `waiterProvider` property in addition to `provider`.
```ts
const apiCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', {
Bucket: 'mybucket',
}).expect(ExpectedResult.objectLike({
KeyCount: 1,
})).waitForAssertions({
interval: cdk.Duration.seconds(30),
totalTimeout: cdk.Duration.minutes(10),
});
```
In the case, `waiterProvider` actually calls to the service/api, so it should have the proper policies.
However a type of a return value of `apiCall` is `IApiCall` interface so that the interface has a `provider` property, `waiterProvider` is not in `IApiCall` but in `AwsApiCall`.
Then it cannot take the policies without casting the following. (`apiCall instanceof AwsApiCall`)
```ts
if (apiCall instanceof AwsApiCall) {
apiCall.waiterProvider?.addToRolePolicy({
Effect: 'Allow',
Action: ['s3:GetObject', 's3:ListBucket'],
Resource: ['*'],
});
}
```
So I add `waiterProvider` to `IApiCall`, so that it can take the policies without casting:
```ts
// if (apiCall instanceof AwsApiCall) {
apiCall.waiterProvider?.addToRolePolicy({
Effect: 'Allow',
Action: ['s3:GetObject', 's3:ListBucket'],
Resource: ['*'],
});
//}
```
In my opinion, I see no negative impact from this.
----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Reverts aws#27844. This change broke the pipeline, pacmak fails with: ``` #STDOUT> /tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon/CDK/IntegTests/Alpha/ApiCallBase.cs(231,77): error CS0115: 'ApiCallBase._Proxy.WaiterProvider': no suitable method found to override [/tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon.CDK.IntegTests.Alpha.csproj] ```
This PR changes to add the
waiterProviderproperty to anIApiCallforawsApiCallin integ-tests-alpha.By default
awsApiCallin integ tests, the AwsApiCall construct will automatically add the correct IAM policies to allow the Lambda function to make the API call. It does this based on the service and api that is provided. In the following example the service is SQS and the api is receiveMessage so it will create a policy with Action: 'sqs:ReceiveMessage'.There are some cases where the permissions do not exactly match the service/api call, for example the S3 listObjectsV2 api. In these cases it is possible to add the correct policy by accessing the
providerobject.On the other hand, there is the case to use
waitForAssertionswhen usingawsApiCallin integ tests. This causesapiCallto have awaiterProviderproperty in addition toprovider.In the case,
waiterProvideractually calls to the service/api, so it should have the proper policies.However a type of a return value of
apiCallisIApiCallinterface so that the interface has aproviderproperty,waiterProvideris not inIApiCallbut inAwsApiCall.Then it cannot take the policies without casting the following. (
apiCall instanceof AwsApiCall)So I add
waiterProvidertoIApiCall, so that it can take the policies without casting:In my opinion, I see no negative impact from this.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license