feat(aws-glue): add glue job and connection constructs by wulfmann · Pull Request #5171 · aws/aws-cdk

wulfmann · 2019-11-25T00:17:14Z

WIP

This is currently a work in progress. Feedback is appreciated.

Rationale

In order to expand the aws-glue construct package, i've begun work on the Job and Connection constructs. I have based my implementation off of the aws-glue Table and Database implementations, as well as other packages in aws-cdk.

Issues

Fixes #5172

Testing

TODO

Documentation

TODO

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

mergify · 2019-11-25T00:17:52Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

wulfmann · 2019-11-25T00:23:46Z

This will likely turn into a very large PR and i'm happy to split this if you wish @skinny85

aws-cdk-automation · 2019-11-25T00:25:36Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-25T00:38:28Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-25T00:48:05Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-25T01:48:28Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-25T01:57:45Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-25T02:08:11Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

skinny85 · 2019-11-25T21:03:26Z

This will likely turn into a very large PR and i'm happy to split this if you wish @skinny85

Yes please! I'm a big believer in small diffs, and the review will be much quicker that way as well :)

mergify · 2019-12-16T12:33:51Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:01:02Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:15Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:38:57Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:41Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:17:47Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

skinny85

Thanks so much for the contribution @wulfmann ! It's a good start, but I have some comments.

skinny85 · 2019-12-17T19:17:24Z

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

+  readonly securityGroupIds?: ec2.ISecurityGroup[];
+
+  /**
+   * @attribute


@attribute is only for properties on the construct (Connection in this case), not on its creation props. Please remove them from everywhere here.

skinny85 · 2019-12-17T19:19:41Z

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

+  /**
+   * @attribute
+   */
+  readonly subnet?: ec2.ISubnet;


I believe what you want here is a standard couple of properties we use in these cases:

readonly vpc: ec2.Vpc; readonly vpcSubnets?: ec2.SubnetSelection;

This should be instead of the subnet property.

skinny85 · 2019-12-17T19:20:59Z

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

+  readonly subnet?: ec2.ISubnet;
+}
+
+export interface IConnectionInput {


The ergonomics of this interface leave a few things to be desired (for example, type requires you to specify an enum that has a single value).

skinny85 · 2019-12-17T19:22:53Z

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

+}
+
+
+export interface ConnectionProps {


We generally prefer to "flatten" creation properties of constructs. Right now, these are pretty much 1:1 with CloudFormation.

I would get rid of both IConnectionInput and IPhysicalRequirements, and add those properties to ConnectionProps.

skinny85 · 2019-12-17T19:24:03Z

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

+
+
+export interface ConnectionProps {
+  readonly catalogId?: string;


Is it actually possible to provide this? Because in the docs below, it says this always has to be account ID.

skinny85 · 2019-12-17T19:26:51Z

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

+  /**
+   * @attribute
+   */
+  readonly connectionInput: IConnectionInput;


I don't see a reason why these 2 properties should be exposed on IConnection, I would leave them out for now.

skinny85 · 2019-12-17T19:27:38Z

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

+
+    this.connectionName = this.getResourceNameAttribute(connectionResource.ref);
+
+    this.node.defaultChild = connectionResource;


Give CfnConnection the ID 'Resource', and you'll be able to get rid of this line.

skinny85 · 2019-12-17T19:31:25Z

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

+  public addConnection(connection: Connection) {
+    this.connections.push(connection);
+  }
+}


Pretty much the same comments here as in the connection.ts file:

@attribute in the wrong places.

Wrong ID of the child L1 (should be 'Resource').

Creation properties are 1:1 with their CloudFormation equivalents - they should be flattened instead.

Too much exposed properties on the IJob interface.

Too many object properties.

skinny85 · 2019-12-17T19:32:22Z

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

+  /**
+   * @attribute
+   */
+  readonly properties: object;


object is a little vague here... Can we make it more precise? Is it a key-value dictionary of strings, for example? ({ [key: string]: string })

wulfmann · 2019-12-18T02:09:12Z

Getting back to this, haven’t touched it in awhile. I plan on scoping this smaller to make it easier to review and will review the changes you suggested.

skinny85 · 2019-12-18T02:21:37Z

I plan on scoping this smaller to make it easier to review and will review the changes you suggested.

Love this attitude 👍

iliapolo · 2020-03-09T11:49:40Z

@wulfmann Wondering about the status of this PR. Are you still maintaining it?

wulfmann · 2020-03-23T20:41:50Z

this is replaced by #6948 as i inadvertently deleted this fork.

add initial connection and job constructs/interfaces

7eab441

wulfmann requested a review from skinny85 as a code owner November 25, 2019 00:17

wulfmann mentioned this pull request Nov 25, 2019

Add Glue Job and Glue Connection Constructs #5172

Closed

2 tasks

wulfmann changed the title ~~Add Glue Job and Glue Connection Constructs~~ feat(aws-glue): Add Glue Job and Glue Connection Constructs Nov 25, 2019

wulfmann changed the title ~~feat(aws-glue): Add Glue Job and Glue Connection Constructs~~ feat(aws-glue): add glue job and connection constructs Nov 25, 2019

Add readme and skeleton tests

d295de6

update tests and fix interfaces

8bb3e9a

cleanup and add connection test

2f64880

add ec2 dependency and update connection and job

ddf15f7

update tests

e02d242

SomayaB assigned skinny85 Nov 25, 2019

skinny85 suggested changes Dec 17, 2019

View reviewed changes

skinny85 requested a review from iliapolo February 6, 2020 22:59

skinny85 assigned iliapolo and unassigned skinny85 Feb 6, 2020

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 12, 2020

wulfmann closed this Mar 23, 2020

wulfmann mentioned this pull request Mar 23, 2020

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

Closed



		export interface ConnectionProps {
		readonly catalogId?: string;


		this.connectionName = this.getResourceNameAttribute(connectionResource.ref);

		this.node.defaultChild = connectionResource;

Conversation

wulfmann commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WIP

Rationale

Issues

Testing

Documentation

Uh oh!

mergify bot commented Nov 25, 2019

Uh oh!

wulfmann commented Nov 25, 2019

Uh oh!

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

Uh oh!

skinny85 commented Nov 25, 2019

Uh oh!

mergify bot commented Dec 16, 2019

Uh oh!

mergify bot commented Dec 16, 2019

Uh oh!

mergify bot commented Dec 16, 2019

Uh oh!

mergify bot commented Dec 16, 2019

Uh oh!

mergify bot commented Dec 16, 2019

Uh oh!

mergify bot commented Dec 16, 2019

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wulfmann commented Dec 18, 2019

Uh oh!

skinny85 commented Dec 18, 2019

Uh oh!

iliapolo commented Mar 9, 2020

Uh oh!

wulfmann commented Mar 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

wulfmann commented Nov 25, 2019 •

edited

Loading