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.

Sign up for GitHub

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

refactor(glue): change physical name to optional #23221

Merged
merged 5 commits into from
Dec 7, 2022
Merged

refactor(glue): change physical name to optional #23221

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a2155e8
refactor(glue): change physicalName to optional
WinterYukky Dec 4, 2022
9e37dfa
test: update integ tests
WinterYukky Dec 4, 2022
be27984
test(glue): set a specify physical name test
WinterYukky Dec 6, 2022
5eeb182
rename
comcalvi Dec 6, 2022
1a489a6
Merge branch 'main' into glue/physical-name-should-optional
mergify[bot] Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 1 addition & 17 deletions packages/@aws-cdk/aws-glue/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ A `SecurityConfiguration` is a set of security properties that can be used by AW

```ts
new glue.SecurityConfiguration(this, 'MySecurityConfiguration', {
securityConfigurationName: 'name',
cloudWatchEncryption: {
mode: glue.CloudWatchEncryptionMode.KMS,
},
Expand All @@ -154,7 +153,6 @@ By default, a shared KMS key is created for use with the encryption configuratio
```ts
declare const key: kms.Key;
new glue.SecurityConfiguration(this, 'MySecurityConfiguration', {
securityConfigurationName: 'name',
cloudWatchEncryption: {
mode: glue.CloudWatchEncryptionMode.KMS,
kmsKey: key,
Expand All @@ -169,9 +167,7 @@ See [documentation](https://docs.aws.amazon.com/glue/latest/dg/encryption-securi
A `Database` is a logical grouping of `Tables` in the Glue Catalog.

```ts
new glue.Database(this, 'MyDatabase', {
databaseName: 'my_database',
});
new glue.Database(this, 'MyDatabase');
```

## Table
Expand All @@ -182,7 +178,6 @@ A Glue table describes a table of data in S3: its structure (column names and ty
declare const myDatabase: glue.Database;
new glue.Table(this, 'MyTable', {
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand All @@ -205,7 +200,6 @@ new glue.Table(this, 'MyTable', {
s3Prefix: 'my-table/',
// ...
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand All @@ -224,7 +218,6 @@ To improve query performance, a table can specify `partitionKeys` on which data
declare const myDatabase: glue.Database;
new glue.Table(this, 'MyTable', {
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand Down Expand Up @@ -256,7 +249,6 @@ property:
declare const myDatabase: glue.Database;
new glue.Table(this, 'MyTable', {
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand Down Expand Up @@ -294,7 +286,6 @@ If you have a table with a large number of partitions that grows over time, cons
declare const myDatabase: glue.Database;
new glue.Table(this, 'MyTable', {
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand Down Expand Up @@ -324,7 +315,6 @@ new glue.Table(this, 'MyTable', {
encryption: glue.TableEncryption.S3_MANAGED,
// ...
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand All @@ -342,7 +332,6 @@ new glue.Table(this, 'MyTable', {
encryption: glue.TableEncryption.KMS,
// ...
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand All @@ -356,7 +345,6 @@ new glue.Table(this, 'MyTable', {
encryptionKey: new kms.Key(this, 'MyKey'),
// ...
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand All @@ -373,7 +361,6 @@ new glue.Table(this, 'MyTable', {
encryption: glue.TableEncryption.KMS_MANAGED,
// ...
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand All @@ -391,7 +378,6 @@ new glue.Table(this, 'MyTable', {
encryption: glue.TableEncryption.CLIENT_SIDE_KMS,
// ...
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand All @@ -405,7 +391,6 @@ new glue.Table(this, 'MyTable', {
encryptionKey: new kms.Key(this, 'MyKey'),
// ...
database: myDatabase,
tableName: 'my_table',
columns: [{
name: 'col1',
type: glue.Schema.STRING,
Expand Down Expand Up @@ -447,7 +432,6 @@ new glue.Table(this, 'MyTable', {
}],
// ...
database: myDatabase,
tableName: 'my_table',
dataFormat: glue.DataFormat.JSON,
});
```
Expand Down
15 changes: 10 additions & 5 deletions packages/@aws-cdk/aws-glue/lib/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ArnFormat, IResource, Resource, Stack } from '@aws-cdk/core';
import { ArnFormat, IResource, Lazy, Names, Resource, Stack } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnDatabase } from './glue.generated';

Expand Down Expand Up @@ -31,8 +31,10 @@ export interface IDatabase extends IResource {
export interface DatabaseProps {
/**
* The name of the database.
*
* @default - generated by CDK.
*/
readonly databaseName: string;
readonly databaseName?: string;

/**
* The location of the database (for example, an HDFS path).
Expand Down Expand Up @@ -86,13 +88,16 @@ export class Database extends Resource implements IDatabase {
*/
public readonly locationUri?: string;

constructor(scope: Construct, id: string, props: DatabaseProps) {
constructor(scope: Construct, id: string, props: DatabaseProps = {}) {
super(scope, id, {
physicalName: props.databaseName,
physicalName: props.databaseName ??
Lazy.string({
produce: () => Names.uniqueResourceName(this, {}).toLowerCase(),
}),
Comment on lines +94 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

why use a Lazy here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to use toLowerCase()?

Copy link
Contributor Author

@WinterYukky WinterYukky Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

why use a Lazy here?

Because physicalName should set in the super. However, when we call the super, we can't use this for Names so we should use a Lazy. Also we can looks this pattern in a other packages (e.g aws-stepfunctions/Activity)

  constructor(scope: Construct, id: string, props: ActivityProps = {}) {
    super(scope, id, {
      physicalName: props.activityName ||
                Lazy.string({ produce: () => this.generateName() }),
    });

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-stepfunctions/lib/activity.ts#L60-L64

Do we have to use toLowerCase()?

Yes, AWS::Glue::Database.DatabaseInput.Name and AWS::Glue::Table.TableInput.Name must be lowercase.

When I use uppercase, I got the error.
image

});

let databaseInput: CfnDatabase.DatabaseInputProperty = {
name: props.databaseName,
name: this.physicalName,
};

if (props.locationUri !== undefined) {
Expand Down
14 changes: 10 additions & 4 deletions packages/@aws-cdk/aws-glue/lib/security-configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as kms from '@aws-cdk/aws-kms';
import * as cdk from '@aws-cdk/core';
import { Lazy, Names } from '@aws-cdk/core';
import * as constructs from 'constructs';
import { CfnSecurityConfiguration } from './glue.generated';

Expand Down Expand Up @@ -114,8 +115,10 @@ export interface JobBookmarksEncryption {
export interface SecurityConfigurationProps {
/**
* The name of the security configuration.
*
* @default - generated by CDK.
*/
readonly securityConfigurationName: string;
readonly securityConfigurationName?: string;

/**
* The encryption configuration for Amazon CloudWatch Logs.
Expand Down Expand Up @@ -184,9 +187,12 @@ export class SecurityConfiguration extends cdk.Resource implements ISecurityConf
*/
public readonly s3EncryptionKey?: kms.IKey;

constructor(scope: constructs.Construct, id: string, props: SecurityConfigurationProps) {
constructor(scope: constructs.Construct, id: string, props: SecurityConfigurationProps = {}) {
super(scope, id, {
physicalName: props.securityConfigurationName,
physicalName: props.securityConfigurationName ??
Lazy.string({
produce: () => Names.uniqueResourceName(this, {}),
}),
});

if (!props.s3Encryption && !props.cloudWatchEncryption && !props.jobBookmarksEncryption) {
Expand Down Expand Up @@ -230,7 +236,7 @@ export class SecurityConfiguration extends cdk.Resource implements ISecurityConf
}

const resource = new CfnSecurityConfiguration(this, 'Resource', {
name: props.securityConfigurationName,
name: this.physicalName,
encryptionConfiguration: {
cloudWatchEncryption,
jobBookmarksEncryption,
Expand Down
13 changes: 9 additions & 4 deletions packages/@aws-cdk/aws-glue/lib/table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import { ArnFormat, Fn, IResource, Names, Resource, Stack } from '@aws-cdk/core';
import { ArnFormat, Fn, IResource, Lazy, Names, Resource, Stack } from '@aws-cdk/core';
import * as cr from '@aws-cdk/custom-resources';
import { AwsCustomResource } from '@aws-cdk/custom-resources';
import { Construct } from 'constructs';
Expand Down Expand Up @@ -83,8 +83,10 @@ export interface TableAttributes {
export interface TableProps {
/**
* Name of the table.
*
* @default - generated by CDK.
*/
readonly tableName: string;
readonly tableName?: string;

/**
* Description of the table.
Expand Down Expand Up @@ -282,7 +284,10 @@ export class Table extends Resource implements ITable {

constructor(scope: Construct, id: string, props: TableProps) {
super(scope, id, {
physicalName: props.tableName,
physicalName: props.tableName ??
Lazy.string({
produce: () => Names.uniqueResourceName(this, {}).toLowerCase(),
}),
});

this.database = props.database;
Expand All @@ -306,7 +311,7 @@ export class Table extends Resource implements ITable {

tableInput: {
name: this.physicalName,
description: props.description || `${props.tableName} generated by CDK`,
description: props.description || `${this.physicalName} generated by CDK`,

partitionKeys: renderColumns(props.partitionKeys),

Expand Down
11 changes: 3 additions & 8 deletions packages/@aws-cdk/aws-glue/test/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ beforeEach( () => {
});

test('default database does not create a bucket', () => {
new glue.Database(stack, 'Database', {
databaseName: 'test_database',
});
new glue.Database(stack, 'Database');

Template.fromStack(stack).templateMatches({
Resources: {
Expand All @@ -27,7 +25,7 @@ test('default database does not create a bucket', () => {
Ref: 'AWS::AccountId',
},
DatabaseInput: {
Name: 'test_database',
Name: 'database',
},
},
},
Expand All @@ -38,7 +36,6 @@ test('default database does not create a bucket', () => {

test('explicit locationURI', () => {
new glue.Database(stack, 'Database', {
databaseName: 'test_database',
locationUri: 's3://my-uri/',
});

Expand All @@ -52,7 +49,7 @@ test('explicit locationURI', () => {
},
DatabaseInput: {
LocationUri: 's3://my-uri/',
Name: 'test_database',
Name: 'database',
},
},
},
Expand All @@ -78,7 +75,6 @@ test('fromDatabase', () => {
test('locationUri length must be >= 1', () => {
expect(() =>
new glue.Database(stack, 'Database', {
databaseName: 'test_database',
locationUri: '',
}),
).toThrow();
Expand All @@ -87,7 +83,6 @@ test('locationUri length must be >= 1', () => {
test('locationUri length must be <= 1024', () => {
expect(() =>
new glue.Database(stack, 'Database', {
databaseName: 'test_database',
locationUri: 'a'.repeat(1025),
}),
).toThrow();
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/aws-glue-connection.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "20.0.0",
"version": "22.0.0",
"files": {
"f076d8bcdf0c659b5885c5698672513188f149a0fe66e8f214a4ff4eaf97fda9": {
"source": {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"20.0.0"}
{"version":"22.0.0"}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/integ.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "20.0.0",
"version": "22.0.0",
"testCases": {
"integ.connection": {
"stacks": [
Expand Down
14 changes: 7 additions & 7 deletions packages/@aws-cdk/aws-glue/test/integ.connection.js.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
{
"version": "20.0.0",
"version": "22.0.0",
"artifacts": {
"Tree": {
"type": "cdk:tree",
"properties": {
"file": "tree.json"
}
},
"aws-glue-connection.assets": {
"type": "cdk:asset-manifest",
"properties": {
Expand Down Expand Up @@ -203,6 +197,12 @@
]
},
"displayName": "aws-glue-connection"
},
"Tree": {
"type": "cdk:tree",
"properties": {
"file": "tree.json"
}
}
}
}
Loading