refactor(glue-alpha): Refactored glue-alpha L2 CDK construct RFC 0497 #32521
Conversation
github-actions
bot
added
the
beginning-contributor
There was a problem hiding this comment.
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.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
natalie-white-aws
changed the title
natalie-white-aws
changed the title
natalie-white-aws
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32521 +/- ##
=======================================
Coverage 81.49% 81.49%
=======================================
Files 224 224
Lines 13755 13755
Branches 2413 2413
=======================================
Hits 11209 11209
Misses 2273 2273
Partials 273 273
Flags with carried forward coverage won't be shown. Click here to find out more.
|
gracelu0
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-community-review
|
Thank you @natalie-white-aws for creating this new PR. Will give it a final pass today and have it merged if I don't have any other major feedback. |
Thanks a lot, let me know if there are any last-minute actions from me. |
|
Went ahead an updated the branch from main to avoid last minute merge conflicts |
|
Woops looks like aws-cli-lib.test broke the build |
|
Yay, the build is back. :) |
|
@GavinZZ It looks like since the last time I brought the branch up to main, someone else has made more changes causing merge conflicts. This is now the second time this has happened so I want to make sure we can merge this quickly once it's in a good state after I resolve it (again). Hopefully I don't have to create a new PR this time, i'll work on it today. I can't resolve the conflicts via the GitHub UI because I don't have write permissions to the core repo. |
|
Looks like it was a typo fix for the comments that caused the merge conflict? Is there any way I can get someone from the CDK team to revert that change and merge this PR since we revamped all of the classes including comments anyway? |
|
@natalie-white-aws I'll revert that PR. There's some issues with our codebuild job so merging a PR is blocked at the moment. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
There was a problem hiding this comment.
Approving this PR as the majority of my feedbacks have been addressed in the old PR #30833. There's some minor comments that are left unaddressed in the old PR and I think it's fine to merge in this PR given the scope of this change is so big and we can update the minor changes in future PRs.
All in all, I want to appreciate @natalie-white-aws for this huge and awesome PR and also appreciate your patience. I know this PR has been sitting here for months.
aws-cdk-automation
removed
the
pr/needs-community-review
|
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). |
|
This pull request has been removed from the queue for the following reason: The pull request can't be updated You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
|
@Mergifyio requeue |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
|
https://github.com/Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
|
This pull request has been removed from the queue for the following reason: The pull request can't be updated You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This pull request has been removed from the queue for the following reason: Pull request #32521 has been dequeued, merge conditions unmatch:
You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
|
Comments on closed issues and PRs are hard for our team to see. |
|
The codebuild CI is successful but somehow keeps showing up as running state here. Bypass the rule and merge the PR. |
Issue # (if applicable)
Implementation of RFC 0497
Reason for this change
Refactored glue-alpha construct to enforce validations by contract and interfaces, improve developer experience, and adhere to best practices. Related PR with merge conflicts and history
Description of changes
Refactored from a single Job class to a pattern of inheritance that removes the need for synth-time validations and sets best practice defaults. Allows for overriding language and Glue versions where applicable, and other job-type specific parameters.
The existing Job and Job Executable monoliths have been decomposed into Job Type and Language specific classes that implement and extend an abstract Job parent class. Developers will be able to see mandatory and optional parameters that apply just to their selected job type and language, rather than having to reference documentation and examples or find out during synth or deploy time that they've selected the wrong configuration.
BREAKING CHANGE: Developers must refactor their existing Job instantiation method calls to choose the right job type and language, and use the new constants static values to define the associated Job configuration settings. See the RFC and/or new README for examples.
Description of how you validated changes
Increased unit test coverage to > 90%, consulted with Glue service team on best practices and sane defaults, updated integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license