-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore(glue-alpha): add 30 missing connection types including SNOWFLAKE #35330
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a typo, PIPEDIVE, but the docs from CFn seem to have the same typo: https://docs.aws.amazon.com/AWSCloudFormation/latest/TemplateReference/aws-properties-glue-connection-connectioninput.html#cfn-glue-connection-connectioninput-connectiontype
I took a look at CloudFormation code and it is a typo on their end too...
| /** | ||
| * Designates a connection to Pipedrive. | ||
| */ | ||
| public static readonly PIPEDIVE = new ConnectionType('PIPEDIVE'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo? Should probably be PIPEDRIVE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all fixed
|
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably correct the typo in our interface. Hopefully CFn corrects this at some point by exposing PIPEDRIVE as a valid option, at which point we can transition over without an API change on our side.
|
This pull request has been removed from the queue for the following reason: Pull request #35330 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:
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. |
| /** | ||
| * Designates a connection to Pipedrive. | ||
| */ | ||
| public static readonly PIPEDIVE = new ConnectionType('PIPEDIVE'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public static readonly PIPEDIVE = new ConnectionType('PIPEDIVE'); | |
| public static readonly PIPEDRIVE = new ConnectionType('PIPEDIVE'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a typo in CloudFormation, we need to maintain the typo, but in our API, we can correct it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be also nice to separate enum names with underscore for better dev experience. For example as
public static readonly ADOBE_ANALYTICS = new ConnectionType('ADOBEANALYTICS');
| * Designates a connection to Google Ads. | ||
| * Designates a connection to Facebook Page Insights. | ||
| */ | ||
| public static readonly GOOGLEADS = new ConnectionType('GOOGLEADS'); | ||
| public static readonly FACEBOOKPAGEINSIGHTS = new ConnectionType('FACEBOOKPAGEINSIGHTS'); | ||
|
|
||
| /** | ||
| * Designates a connection to Google Sheets. | ||
| * Designates a connection to Freshdesk. | ||
| */ | ||
| public static readonly GOOGLESHEETS = new ConnectionType('GOOGLESHEETS'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a confusion on this part, where existing enum seems like to be deleted, but it is only moved to a different place. Could you keep the order on this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR sorts all of them alphabetically.
|
@Mergifyio refresh |
✅ Pull request refreshed |
Issue: Long compound connection type names were difficult to read without underscores Solution Implemented: Updated all multi-word connection types to use underscores for improved readability while maintaining CloudFormation compatibility. Connection Types Updated:
Single-Word Types Preserved:The following connection types remained unchanged as they are single words or established brand names: Let me know if I missed anything. |
|
@pahud Thanks, those proposed enum names looking good. I assume you did not push them yet |
Pushed. Logged an internal ticket to CFN team for the |
|
Added do not merge label, until we get a response from service team |
|
Closing in favor of a new PR with updated enum values |
|
This issue has been reopened and is now available for discussion. |
|
still pending for internal clarification. |
Issue # (if applicable)
Closes #35318.
Reason for this change
AWS Glue supports 58+ connection types in CloudFormation, but the aws-glue-alpha CDK module only exposed 28 of them as static properties in the ConnectionType class. This left 30 connection types missing, including SNOWFLAKE (requested in #35318) and many other popular enterprise platforms like Microsoft Teams, GitLab, Mailchimp, PayPal, QuickBooks, and other major SaaS platforms.
This forces users to use the less convenient workaround
new ConnectionType('TYPE_NAME')instead of the standard static property pattern used for supported connection types likeConnectionType.JDBCorConnectionType.MONGODB.This enhancement provides comprehensive coverage of all AWS Glue supported connection types, offering a complete and consistent developer experience across all supported platforms.
Description of changes
This change adds 30 missing static readonly properties to the
ConnectionTypeclass in the aws-glue-alpha module, bringing full parity with AWS CloudFormation's supported connection types:New Connection Types Added:
Implementation Details:
public static readonly NAME = new ConnectionType('NAME');The implementation follows the exact pattern established by recent commit 44360cc, which added 20+ connection types using the same approach. The change enables users to write idiomatic CDK code for all AWS Glue supported platforms:
Before (workaround):
After (convenient static property):
Describe any new or updated permissions being added
N/A - This change only adds a static readonly property to an existing class. No new IAM permissions or resource access patterns are introduced.
Description of how you validated changes
Unit tests: Added comprehensive test coverage in
packages/@aws-cdk/aws-glue-alpha/test/connection.test.ts:ConnectionType.NAMEstatic propertyConnectionTypevaluesIntegration tests: Not required - this is a simple static property addition with no behavioral changes. Recent precedent (commit 44360cc) added 20+ connection types without integration tests, following the same pattern.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license