-
Notifications
You must be signed in to change notification settings - Fork 4k
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): cannot publish a file without extension #30597
Changes from 2 commits
9449634
98a0371
5c443ad
69889ec
ecee169
9f5adb5
ca501e7
12bb8a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -278,9 +278,10 @@ export class AssetStaging extends Construct { | |||||
*/ | ||||||
private stageByCopying(): StagedAsset { | ||||||
const assetHash = this.calculateHash(this.hashType); | ||||||
const stagedPath = this.stagingDisabled | ||||||
const targetPath = this.stagingDisabled | ||||||
? this.sourcePath | ||||||
: path.resolve(this.assetOutdir, renderAssetFilename(assetHash, getExtension(this.sourcePath))); | ||||||
const stagedPath = this.renderStagedPath(this.sourcePath, targetPath); | ||||||
|
||||||
if (!this.sourceStats.isDirectory() && !this.sourceStats.isFile()) { | ||||||
throw new Error(`Asset ${this.sourcePath} is expected to be either a directory or a regular file`); | ||||||
|
@@ -338,7 +339,10 @@ export class AssetStaging extends Construct { | |||||
// Calculate assetHash afterwards if we still must | ||||||
assetHash = assetHash ?? this.calculateHash(this.hashType, bundling, bundledAsset.path); | ||||||
|
||||||
const stagedPath = path.resolve(this.assetOutdir, renderAssetFilename(assetHash, bundledAsset.extension)); | ||||||
const stagedPath = this.renderStagedPath( | ||||||
bundledAsset.path, | ||||||
path.resolve(this.assetOutdir, renderAssetFilename(assetHash, bundledAsset.extension)), | ||||||
); | ||||||
|
||||||
this.stageAsset(bundledAsset.path, stagedPath, 'move'); | ||||||
|
||||||
|
@@ -388,7 +392,7 @@ export class AssetStaging extends Construct { | |||||
} | ||||||
|
||||||
// Moving can be done quickly | ||||||
if (style == 'move') { | ||||||
if (style === 'move') { | ||||||
fs.renameSync(sourcePath, targetPath); | ||||||
return; | ||||||
} | ||||||
|
@@ -511,6 +515,15 @@ export class AssetStaging extends Construct { | |||||
throw new Error('Unknown asset hash type.'); | ||||||
} | ||||||
} | ||||||
|
||||||
private renderStagedPath(sourcePath: string, targetPath: string): string { | ||||||
// Add a suffix to the asset file name | ||||||
// because when a file without extension is specified, the source directory name is the same as the staged asset file name. | ||||||
if (this.hashType === AssetHashType.SOURCE && path.dirname(sourcePath) === targetPath) { | ||||||
targetPath = targetPath + 'noext'; | ||||||
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.
Suggested change
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. Changed suffix to |
||||||
} | ||||||
return targetPath; | ||||||
} | ||||||
} | ||||||
|
||||||
function renderAssetFilename(assetHash: string, extension = '') { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,9 @@ export class AssetManifestBuilder { | |
public defaultAddFileAsset(stack: Stack, asset: FileAssetSource, target: AssetManifestFileDestination) { | ||
validateFileAssetSource(asset); | ||
|
||
// If the fileName begins with 'asset.', remove it here | ||
// because when a file without extension is added to the manifest, the path.extension() will return an file hash. | ||
// ex) path.extension('asset.dc5ce447844') => 'dc5ce447844' | ||
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 comment seems to not match what the code is doing? Or does it? 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. Deleted comment. |
||
const extension = | ||
asset.fileName != undefined ? path.extname(asset.fileName) : ''; | ||
const objectKey = | ||
|
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.
Does this even on the
hashType
? It seems that ifdirname(sourcePath) === targetPath
we always have a problem?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.
Changed the conditional to
this.hashType !== AssetHashType.OUTPUT
and added the reason to the comment.9f5adb5