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.

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

chore(parameters): Use AppConfig L2 constructs for integration tests #2524

Merged
merged 4 commits into from
May 15, 2024

Conversation

daschaa
Copy link
Contributor

@daschaa daschaa commented May 14, 2024

Summary

Changes

This PR implements the L2 constructs for the integration tests in the parameters package. Closes #2396

Issue number: #2396


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@boring-cyborg boring-cyborg bot added the tests PRs that add or change tests label May 14, 2024
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label May 14, 2024
@daschaa daschaa changed the title chore(parameter): Use AppConfig L2 constructs for integration tests chore(parameters): Use AppConfig L2 constructs for integration tests May 14, 2024
@dreamorosi
Copy link
Contributor

Preliminary run for the integration tests is green: https://github.com/aws-powertools/powertools-lambda-typescript/actions/runs/9086242800

Whenever you want me to run it again, just tag me and I'll do it (it's manual for PR coming from forks).

@dreamorosi
Copy link
Contributor

Like before, if you could leave a comment under the original issue I'll assign it to you - so that everyone knows the issue is taken.

@daschaa
Copy link
Contributor Author

daschaa commented May 14, 2024

@dreamorosi Thanks for your fast feedback, I really appreciate it!

I just marked this PR as a draft because I hadn't got the time to run the integration tests, I just compared the synthed output and tried to remodel the current behavior.

So did you let the integration tests run with the current state of this PR? Can I mark it as "Ready for Review" already?

@dreamorosi
Copy link
Contributor

Yes, I ran it with the latest commit around the time I left my comment.

You can mark it as ready whenever you're ready - no rush from my side.

@daschaa daschaa marked this pull request as ready for review May 14, 2024 21:59
@daschaa daschaa requested review from a team as code owners May 14, 2024 21:59
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Left one minor comment here. Once it's addressed I'll run the integration tests again and if they're green I'll merge it.

Thanks again for your contribution!

Copy link

sonarcloud bot commented May 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dreamorosi dreamorosi self-requested a review May 15, 2024 17:00
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

I have ran the integration tests once again and they're green.

Thank you so much for working on this PR!

@dreamorosi dreamorosi merged commit 2035c93 into aws-powertools:main May 15, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: adopt AppConfig L2 constructs in Parameters integration tests
2 participants