Skip to content

chore: migrate njlynch's modules to assertions#18204

Merged
mergify[bot] merged 26 commits intomasterfrom
route53-assertions
Jan 20, 2022
Merged

chore: migrate njlynch's modules to assertions#18204
mergify[bot] merged 26 commits intomasterfrom
route53-assertions

Conversation

@kaizencc
Copy link
Copy Markdown
Contributor

@kaizencc kaizencc commented Dec 28, 2021

This PR is complete when these 16 modules are accounted for:

  • route53
  • route53-targets
  • route53-patterns
  • elasticloadbalancingv2
  • elasticloadbalancingv2-targets
  • elasticloadbalancingv2-actions
  • elasticloadbalancing
  • cloudfront
  • cloudfront-origins
  • secretsmanager
  • ec2
  • certificates-manager
  • sns-subscriptions
  • sqs
  • ssm
  • kms

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

@kaizencc kaizencc requested a review from njlynch December 28, 2021 17:40
@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Dec 28, 2021

@github-actions github-actions bot added the @aws-cdk/aws-route53 Related to Amazon Route 53 label Dec 28, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 28, 2021
@kaizencc kaizencc changed the title chore(route53): migrate tests to assertions chore: migrate njlynch's modules to assertions Dec 28, 2021
@github-actions github-actions bot added the @aws-cdk/assertions Related to the @aws-cdk/assertv2 package label Dec 28, 2021
@kaizencc kaizencc added pr/do-not-merge This PR should not be merged at this time. and removed @aws-cdk/assertions Related to the @aws-cdk/assertv2 package labels Dec 28, 2021
@github-actions github-actions bot added the @aws-cdk/assertions Related to the @aws-cdk/assertv2 package label Dec 28, 2021
Copy link
Copy Markdown
Contributor Author

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Can't approve since I started this PR, but this is my stamp of approval :).

Just the one comment/question about Match.not...

});

expect(stack).not.toHaveResourceLike('AWS::ElasticLoadBalancingV2::TargetGroup', {
const matches = Template.fromStack(stack).findResources('AWS::ElasticLoadBalancingV2::TargetGroup', {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The original intention here is to do:

Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::TargetGroup', Match.not({
  ...
});

@rix0rrr seems to hate this but I think it's reasonable; Match.not simply inverts the result so it succeeds with any matching failure. Curious if this is something you looked at and actively avoided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I looked at it, it just felt... wrong? And it's not entirely equivalent; for example, the above fails if there are no TargetGroups. Maybe I should be using it, as I guess it's the more idiomatic approach for assertions, but something about the way the API currently expresses itself makes me wrinkle my nose a bit. I'd like to think of a better approach, but also didn't want to block this migration on it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the response. The equivalence issue is true, but to play devils advocate, I don't think the Assertions API was developed with the idea of exact 1-1 matching with assert-internal. But I hear what both you and Rico are saying.

@njlynch njlynch removed the pr/do-not-merge This PR should not be merged at this time. label Jan 19, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 19, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

]));


expect(vpc.node.metadataEntry).toContainEqual({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kaizen3031593 this looks like the solution to what we were talking about yesterday.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call. Although I am about to write a PR for messages support that would look something like this:

AssertAnnotations.fromStack(stack).hasWarning('*', {
  entry: {
    type: 'aws:cdk:warning',
    data: "fromVpcAttributes...",
  },
});

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 20, 2022

Thank you for contributing! Your pull request will be updated from master 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
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 3dd2649
  • 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 455d0b3 into master Jan 20, 2022
@mergify mergify bot deleted the route53-assertions branch January 20, 2022 13:54
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 20, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
This PR is complete when these 16 modules are accounted for:

- [x] route53
- [x] route53-targets
- [x] route53-patterns
- [x] elasticloadbalancingv2
- [x] elasticloadbalancingv2-targets
- [x] elasticloadbalancingv2-actions
- [x] elasticloadbalancing
- [x] cloudfront
- [x] cloudfront-origins
- [x] secretsmanager
- [x] ec2
- [x] certificates-manager
- [x] sns-subscriptions
- [x] sqs
- [x] ssm
- [x] kms

----

*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

Labels

@aws-cdk/assertions Related to the @aws-cdk/assertv2 package @aws-cdk/aws-route53 Related to Amazon Route 53 contribution/core This is a PR that came from AWS.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants