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

Jump to bottom

[Glue] Empty string is not considered a proper value #5763

Closed
melgenek opened this issue Jan 12, 2020 · 3 comments · Fixed by #5783
Closed

[Glue] Empty string is not considered a proper value #5763

melgenek opened this issue Jan 12, 2020 · 3 comments · Fixed by #5783
Assignees
@skinny85
Labels
@aws-cdk/aws-glue Related to AWS Glue bug This issue is a bug. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.

Comments

@melgenek
Copy link
Contributor

melgenek commented Jan 12, 2020 •
edited
Loading

An empty string is a Falsy js value.
It is not possible to set an empty s3Prefix for Glue Table https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-glue/lib/table.ts#L239
This issue also does not give to have an empty location in the Glue Database https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-glue/lib/database.ts#L93

Reproduction Steps

In browser execute js:

"" || "/data"

the result is "/data"

Error Log

Environment

  • CLI Version : 1.20.0
  • Framework Version: 1.20.0
  • OS : GNU/Linux
  • Language : Java

Other

A similar issue has already been closed once #801
There is a related issue that could be closed if there was a possibility to pass empty strings #5268

This is 🐛 Bug Report
@melgenek melgenek added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 12, 2020
@skinny85
Copy link
Contributor

Thanks for opening the issue @melgenek . Would you consider opening us a PR changing this? It should be a simple fix.

@skinny85 skinny85 self-assigned this Jan 13, 2020
@skinny85 skinny85 added good first issue Related to contributions. See CONTRIBUTING.md @aws-cdk/aws-glue Related to AWS Glue and removed needs-triage This issue or PR still needs to be triaged. labels Jan 13, 2020
melgenek pushed a commit to melgenek/aws-cdk that referenced this issue Jan 13, 2020
fix(glue): Empty string is not undefined. (aws#5763)
5b11289 
When the Glue Table s3Prefix is an empty string, it should not be replaced with a default value.
@melgenek melgenek mentioned this issue Jan 13, 2020
Merged
@melgenek
Copy link
Contributor Author

I have created a PR that fixes the s3Prefix #5783.
During implementation, I figured out that #5268 is actually a better fit for locationUri problem because an empty locationUri is not allowed by cloudformation so the param should not be passed at all.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jan 13, 2020
@SomayaB
Copy link
Contributor

SomayaB commented Jan 13, 2020

Thanks for opening those PRs @melgenek, much appreciated! 👍

skinny85 pushed a commit that referenced this issue Jan 13, 2020
@melgenek @skinny85
fix(glue): empty string in Table.s3prefix is not undefined (#5783)
18e15de 
When the Glue Table's s3Prefix property is an empty string,
it should not be replaced with the default value.

Fixes #5763
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@skinny85 skinny85

Labels
@aws-cdk/aws-glue Related to AWS Glue bug This issue is a bug. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(glue): An empty string is not the same as undefined. melgenek/aws-cdk
3 participants
@skinny85 @melgenek @SomayaB