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

fix(events): archive construct does not have defaultChild set #21345

Merged
merged 6 commits into from
Aug 1, 2022
Merged

fix(events): archive construct does not have defaultChild set #21345

merged 6 commits into from
Aug 1, 2022

Conversation

daschaa
Copy link
Contributor

@daschaa daschaa commented Jul 27, 2022

Fixes #21263

Adds id 'Default' to CfnArchive in Archive construct.

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • 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

@gitpod-io
Copy link

gitpod-io bot commented Jul 27, 2022

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jul 27, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team July 27, 2022 20:31
TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Jul 27, 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.

This feels(?) like a breaking change but I can't actually tell if it is. Part of the problem is that no integration tests were written for this module. Even though this is a fix, not a feat, I think we need an integration test here.

@daschaa
Copy link
Contributor Author

daschaa commented Jul 28, 2022

@TheRealAmazonKendra Yeah maybe you are right.. Do you think about some logical ID matching stuff out in the wild? Should I introduce a feature flag for this then? It would feel weird to make a feature flag for a fix tbh.. What would be the best way to keep backwards compatibility? I will definitely add Integration tests!

And btw: is this even the best way to define the CfnArchive as defaultChild? Or is there a better way?

rix0rrr
rix0rrr previously requested changes Jul 28, 2022
View reviewed changes
packages/@aws-cdk/aws-events/lib/archive.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed stale reviews from TheRealAmazonKendra and rix0rrr July 28, 2022 13:45

Pull request has been modified.

@daschaa
Copy link
Contributor Author

daschaa commented Jul 28, 2022

Sorry @rix0rrr - I have now changed the code and implemented your suggestion.

TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Jul 28, 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.

Just one quick comment on the unit test and I'd still like to see an integ tests added.

packages/@aws-cdk/aws-events/test/archive.test.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 1, 2022 06:32

Pull request has been modified.

@daschaa
Copy link
Contributor Author

daschaa commented Aug 1, 2022

@TheRealAmazonKendra Thanks for your feedback :) I have added an integration test and changed the unit test according to your review.

TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Aug 1, 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.

Thank you for adding the tests. We just need the integration test to use the new IntegTest construct, per the integration testing guide.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 1, 2022 17:12

Pull request has been modified.

@daschaa
Copy link
Contributor Author

daschaa commented Aug 1, 2022

@TheRealAmazonKendra I have added the IntegTest structure like in the example of the markdown file you provided. Is this correct?

TheRealAmazonKendra
TheRealAmazonKendra approved these changes Aug 1, 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 great!

@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3eae89d
  • 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 de7d825 into aws:main Aug 1, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2022

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

@daschaa daschaa deleted the aws-events_archive_default_child branch August 1, 2022 20:02
josephedward pushed a commit to josephedward/aws-cdk that referenced this pull request Aug 30, 2022
@daschaa @josephedward
fix(events): archive construct does not have defaultChild set (aws#21345
628c0a1 
)

Fixes aws#21263 

Adds id 'Default' to CfnArchive in Archive construct.

----

### All Submissions:

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

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheRealAmazonKendra TheRealAmazonKendra TheRealAmazonKendra approved these changes

@rix0rrr rix0rrr rix0rrr left review comments

Assignees
No one assigned
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws-events: Archive construct does not have defaultChild set
4 participants
@daschaa @aws-cdk-automation @rix0rrr @TheRealAmazonKendra