-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(s3-assets): throw if path property is empty #29425
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -143,11 +143,18 @@ test('"readers" or "grantRead" can be used to grant read permissions on the asse | |||||||
}); | ||||||||
}); | ||||||||
|
||||||||
test('fails if path is empty', () => { | ||||||||
const stack = new cdk.Stack(); | ||||||||
expect(() => new Asset(stack, 'MyDirectory', { | ||||||||
path: '', | ||||||||
})).toThrow(/Asset path cannot be empty/); | ||||||||
}); | ||||||||
|
||||||||
test('fails if directory not found', () => { | ||||||||
const stack = new cdk.Stack(); | ||||||||
expect(() => new Asset(stack, 'MyDirectory', { | ||||||||
path: '/path/not/found/' + Math.random() * 999999, | ||||||||
})).toThrow(); | ||||||||
})).toThrow(/Cannot find asset/); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this just throws inherently, correct? Possible to catch this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar to aws-cdk/packages/aws-cdk-lib/core/lib/asset-staging.ts Lines 174 to 176 in e63c777
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just wanted to refine the existing test to help differentiate between the two errors, since I was adding a new one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see it now 👍🏼 |
||||||||
}); | ||||||||
|
||||||||
test('multiple assets under the same parent', () => { | ||||||||
|
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 might also be a good idea to check that
cdk.out
is excluded from the asset source, asCode.fromAsset('.')
would cause the same reported issue.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.
+1 on this
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.
Good call out, I'm good with the changes in this PR so I'll approve for now but feel free to add that check in a separate PR.