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

feat(integ-tests): optionaly install latest aws SDK when using integ-test AwsApiCall #22443

Closed
wants to merge 0 commits into from
Closed

feat(integ-tests): optionaly install latest aws SDK when using integ-test AwsApiCall #22443

wants to merge 0 commits into from

Conversation

nbaillie
Copy link
Contributor

@nbaillie nbaillie commented Oct 10, 2022 •
edited
Loading

This PR enabled optional install of the latest AWS SDK when using AwsApiCall.

This helps when testing new features that are not supported by the default sdk in lambda runtime.

The runtime approach is similar to : custom-resource

Readme updated and test added.

Currently does not address directly the fact that the call will happen on create and then again on delete. but looks like this is addressed here.

Feature is in relation to, but not listed tracker

All Submissions:

New Features

  • Integ-test added

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

@gitpod-io
Copy link

gitpod-io bot commented Oct 10, 2022

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Oct 10, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team October 10, 2022 14:05
@github-actions github-actions bot added the p2 label Oct 10, 2022
aws-cdk-automation
aws-cdk-automation previously requested changes Oct 10, 2022
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.

@nbaillie nbaillie changed the title nbaillie/integ-test-latest-sdk feature(integ-tests): optionaly install latest aws SDK when using integ-test AwsApiCall Oct 10, 2022
@nbaillie nbaillie changed the title feature(integ-tests): optionaly install latest aws SDK when using integ-test AwsApiCall feat(integ-tests): optionaly install latest aws SDK when using integ-test AwsApiCall Oct 10, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 10, 2022 15:54

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

@nbaillie
Copy link
Contributor Author

Build failed due to salt on custom resource params

added a context key check to allow a static salt to be set. used: '@aws-cdk/core:assetHashSalt' as it has been used before, might want to use a unique key. but not sure.

@nbaillie nbaillie marked this pull request as ready for review October 10, 2022 19:29
rix0rrr
rix0rrr reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Thanks Neil! Looks good! I want to run this past Cory though to check out the need for the salt context parameter, that's not what that should be used for methinks.

packages/@aws-cdk/integ-tests/lib/assertions/sdk.ts Outdated
Comment on lines 65 to 85
const salt = this.node.tryGetContext(ASSET_SALT_CONTEXT_KEY);

Copy link
Contributor

Choose a reason for hiding this comment

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

Why this? It's not what that key was intended for.

But I think I do see: is it because otherwise one cannot get a stable snapshot of the template?

If so, this needs @corymhall in here, who's currently the deepest into integ testing. The integ testing facilities should automatically be producing consistent values whenever they know their snapshot is going to be taken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes your right... i thought i might need to use another way or use another key..
It is due to the snapshot test stability.

stems from the salt = Date.now()

perhaps if we could have some way to make code aware it was in integ-runner other than context. Something like jest.spyOn() to help make Date.now() static was another thought.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for this I would rather make the runner aware of the assertions. On diff it could just ignore the salt (like we do for asset hashes).

@nbaillie nbaillie mentioned this pull request Oct 11, 2022
Merged
3 tasks
mergify bot pushed a commit that referenced this pull request Oct 27, 2022
@nbaillie
feat(ec2): Vpc supports allocating CIDR from AWS IPAM (#22458)
7ed9cd1 
Allows Vpc to Use [Aws IPAM](https://docs.aws.amazon.com/vpc/latest/ipam/what-it-is-ipam.html) for Ip address assignment:

```ts
import { IpAddresses } from '@aws-cdk/aws-ec2';

declare const pool: ec2.CfnIPAMPool;

new ec2.Vpc(stack, 'TheVPC', {
  ipAddresses: ec2.IpAddresses.awsIpamAllocation({
    ipv4IpamPoolId: pool.ref,
    ipv4NetmaskLength: 18,
    defaultSubnetIpv4NetmaskLength: 24
  })
});
```

This is useful for enterprise users that wish to adopt the benefits of centralised IP address management.

It introduces `ipAddresses` property to allow the new configuration.

----

Thanks to @rix0rrr for support on this.

---

closes #21333

----

#22443 - Issue adds a fix to allow the clean up of the AWS Ipam resource used in ingeg-test testing. Would be better to implement something like this later. for now disclaimer added to integ-test clean up needed on Ipam.

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### New Features

* [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TheRealAmazonKendra
TheRealAmazonKendra requested changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Looks like the last comments on this were requesting changes so putting this into changes requested to accurately reflect the status.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@nbaillie nbaillie closed this Nov 28, 2022
@nbaillie nbaillie force-pushed the nbaillie/integ-test-latest-sdk branch from b5b08ed to 437c21f Compare November 28, 2022 20:34
aws-cdk-automation
aws-cdk-automation requested changes Nov 28, 2022
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.

@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.

@nbaillie
Copy link
Contributor Author

Still working to update this.. will take a bit longer than i had hoped right now.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corymhall corymhall corymhall left review comments

@rix0rrr rix0rrr rix0rrr left review comments

@TheRealAmazonKendra TheRealAmazonKendra TheRealAmazonKendra requested changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation requested changes

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nbaillie @aws-cdk-automation @rix0rrr @corymhall @TheRealAmazonKendra