feat(aws-glue): add glue connection L2 Constructs #6948

wulfmann · 2020-03-23T20:40:59Z

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

wulfmann · 2020-03-23T20:43:48Z

@skinny85 this is ready for a review. I made the changes you suggested on #5171

wulfmann · 2020-03-23T20:45:17Z

packages/@aws-cdk/aws-glue/lib/connection.ts

+  constructor(scope: Construct, id: string, props: ConnectionProps) {
+    super(scope, id);
+
+    const subnetId = props.vpcSubnets && props.vpc ? props.vpc.selectSubnets(props.vpcSubnets).subnetIds[0] : undefined;


@skinny85 I wasn't entirely sure what you meant on the other PR. Is this what you intended for the subnets? I have the same question for line 106.

It should be:

const subnetId = props.vpc ? props.vpc.selectSubnets(props.vpcSubnets).subnetIds[0] : undefined;

skinny85

Thanks for the contribution @wulfmann ! I have some comments, but we're getting there!

packages/@aws-cdk/aws-glue/lib/connection.ts

packages/@aws-cdk/aws-glue/test/test.connection.ts

skinny85 · 2020-03-24T00:23:12Z

packages/@aws-cdk/aws-glue/test/test.connection.ts

+        connectionProperties: {},
+    });
+
+    expect(stack).toMatch({


I think you want expect(stack).toHaveResourceLike(...).... right now, these tests don't actually verify anything.

Yea im still working on tests.

packages/@aws-cdk/aws-glue/test/test.connection.ts

packages/@aws-cdk/aws-glue/lib/connection.ts

Pull request has been modified.

…-connection

skinny85 · 2020-03-24T22:10:24Z

packages/@aws-cdk/aws-glue/test/connection.test.ts

+import * as glue from '../lib';
+import * as ec2 from '@aws-cdk/aws-ec2';
+
+export = {


Uhm. Maybe I did a mental shortcut saying "that's all you needed". Jest tests look a tiny bit different than Nodeunit tests. So, this file would be something like:

describe('Glue Connection', () => { test('can be created without a VPC', () => { // ... }); test('can be created with a VPC', () => { // ... }); });

skinny85 · 2020-03-25T18:09:45Z

About the Jest tests - my apologies, I forgot this module already has some Nodeunit tests. In this case (because you can't mix Nodeunit and Jest in the same CDK package), just make the new tests in Nodeunit as well.

Apologies for the churn!

skinny85 · 2020-03-25T18:38:31Z

BTW, we're investigating why the build fails with

 error TS0: Exported APIs cannot use un-exported type @aws-cdk/aws-glue.ConnectionInputTypes.JDBC

We know it's from the single-value enum, but it shouldn't really be happening. Will update when we know more.

skinny85

Just removing from my to-do list.

mergify · 2020-03-25T18:39:31Z

Once all the requested changes have been addressed, and the PR is ready for another review, remember to dismiss the review.

TypeScript represents enums similar to union types, and single-valued enums are "simplified" to the sole member (the TypeChecker fiercely refuses to give a handle to the actual `enum` type). This resulted in `jsii` incorrectly tagging the type at usage sites. This commit adds the necessary infrastructure to detect single-valued enums and tweak the FQN generation to obtain the correct result. A new test was added to validate this whole endeavor works correctly even when the single-valued enum is within a submodule (which adds even more complexity to the mix). References: aws/aws-cdk#6712 aws/aws-cdk#6948

skinny85 · 2020-08-31T21:29:59Z

@wulfmann any updates here? Are you ready for another round of reviews, or still working on it? (The single-value enum thing should be resolved now)

wulfmann · 2020-08-31T22:36:07Z

I will check this first thing tomorrow and see if we can get this resolved!

humanzz · 2020-09-18T13:49:46Z

Hi everyone,
Just checking on the status of this as my team is really interested in Job constructs for glue.

skinny85 · 2020-09-18T17:23:15Z

Still waiting for @wulfmann on this one 🙂

humanzz · 2020-09-22T10:31:25Z

packages/@aws-cdk/aws-glue/lib/connection.ts

+   * The type of the connection. Currently, only JDBC is supported;
+   * SFTP is not supported.
+   */
+  JDBC = 'JDBC',


Seems like there are more types currently supported https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-glue-connection-connectioninput.html

humanzz · 2020-09-22T10:36:50Z

@skinny85 Any chance/way to help completing this PR if @wulfmann is not available to wrap it?

Pull request has been modified.

wulfmann · 2020-09-29T01:05:06Z

Finally back to wrapping this up. Sorry to everyone waiting on this.

wulfmann · 2020-09-29T01:19:37Z

I've updated to the current branch of upstream and cleaned up some of the missing things. I should be able to wrap the tests up tomorrow so we can at least get the L2 connection construct merged.

aws-cdk-automation · 2020-09-29T15:00:14Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 45afa5b
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

skinny85

@wulfmann let me know when this is ready for review!

(Putting in "Request changes" to remove it from my ToDo list)

humanzz · 2020-12-14T10:03:34Z

@wulfmann any updates here?

skinny85 · 2021-06-22T16:19:41Z

I'm closing this one due to inactivity, please comment if you want it re-opened.

update connection

69bb59b

This was referenced Mar 23, 2020

feat(aws-glue): add glue job and connection constructs #5171

Closed

Add Glue Job and Glue Connection Constructs #5172

Closed

wulfmann changed the title ~~feat(aws-glue): add glue job and connection constructs~~ feat(aws-glue): add glue connection L2 Constructs Mar 23, 2020

Merge branch 'master' into aws-glue-connection

36c92e9

wulfmann commented Mar 23, 2020

View reviewed changes

skinny85 self-assigned this Mar 23, 2020

skinny85 previously requested changes Mar 24, 2020

View reviewed changes

wulfmann added 3 commits March 24, 2020 08:30

update tests to jest and cleanup

2ef4f4e

fix conflict

6f380ce

Revert yarn.lcok:

cc31eb2

wulfmann added 3 commits March 24, 2020 08:35

Reset yarn.lock

8a38354

Merge branch 'master' of https://github.com/aws/aws-cdk into aws-glue…

0590ff0

…-connection

fix import

96df1df

skinny85 reviewed Mar 24, 2020

View reviewed changes

skinny85 previously requested changes Mar 25, 2020

View reviewed changes

RomainMuller mentioned this pull request Mar 26, 2020

fix(jsii): single-valued enums are not processed correctly aws/jsii#1406

Merged

bweigel mentioned this pull request Apr 12, 2020

feat(redshift): add initial L2 Redshift construct #5730

Merged

4 tasks

humanzz reviewed Sep 22, 2020

View reviewed changes

fix upstream

a099a9b

Add new connection types

2997ca5

wulfmann added 4 commits September 28, 2020 20:13

remove bad tools commit

d017d0c

cleanup bad config

ee9041d

remove jest key

c8cceef

start doc change

b2bc515

wulfmann added 4 commits September 29, 2020 09:46

get first test working

3201d7d

update yarn test

53de63e

Add ec2 dep

808c88a

Merge branch 'master' into aws-glue-connection

45afa5b

skinny85 suggested changes Oct 14, 2020

View reviewed changes

skinny85 closed this Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-glue): add glue connection L2 Constructs #6948

feat(aws-glue): add glue connection L2 Constructs #6948

wulfmann commented Mar 23, 2020

wulfmann commented Mar 23, 2020

wulfmann Mar 23, 2020

skinny85 Mar 24, 2020

skinny85 left a comment

skinny85 Mar 24, 2020

wulfmann Mar 24, 2020

skinny85 Mar 24, 2020 •

edited

Loading

skinny85 commented Mar 25, 2020

skinny85 commented Mar 25, 2020

skinny85 left a comment

mergify bot commented Mar 25, 2020

skinny85 commented Aug 31, 2020

wulfmann commented Aug 31, 2020

humanzz commented Sep 18, 2020

skinny85 commented Sep 18, 2020

humanzz Sep 22, 2020

wulfmann Sep 29, 2020

humanzz commented Sep 22, 2020

wulfmann commented Sep 29, 2020

wulfmann commented Sep 29, 2020

aws-cdk-automation commented Sep 29, 2020

skinny85 left a comment

humanzz commented Dec 14, 2020

skinny85 commented Jun 22, 2021

feat(aws-glue): add glue connection L2 Constructs #6948

feat(aws-glue): add glue connection L2 Constructs #6948

Conversation

wulfmann commented Mar 23, 2020

wulfmann commented Mar 23, 2020

wulfmann Mar 23, 2020

Choose a reason for hiding this comment

skinny85 Mar 24, 2020

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Mar 24, 2020

Choose a reason for hiding this comment

wulfmann Mar 24, 2020

Choose a reason for hiding this comment

skinny85 Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

skinny85 commented Mar 25, 2020

skinny85 commented Mar 25, 2020

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 25, 2020

skinny85 commented Aug 31, 2020

wulfmann commented Aug 31, 2020

humanzz commented Sep 18, 2020

skinny85 commented Sep 18, 2020

humanzz Sep 22, 2020

Choose a reason for hiding this comment

wulfmann Sep 29, 2020

Choose a reason for hiding this comment

humanzz commented Sep 22, 2020

wulfmann commented Sep 29, 2020

wulfmann commented Sep 29, 2020

aws-cdk-automation commented Sep 29, 2020

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

humanzz commented Dec 14, 2020

skinny85 commented Jun 22, 2021

skinny85 Mar 24, 2020 •

edited

Loading