feat(maven): Support BOM files in maven target - #270
Conversation
| const revision = 'r3v1s10n'; | ||
|
|
||
| await mvnTarget.publish(version, revision); | ||
| await mvnTarget.publish('1.0.0', revision); |
There was a problem hiding this comment.
Why change to directly passing in the string vs. a variable like before?
There was a problem hiding this comment.
The version isn't being used anywhere else, so IMO removes a bit of noise (like in others upload calls). Do you think it's easier to read by leaving the variable?
There was a problem hiding this comment.
No I have no strong opinions, just wondering. I agree it helps reduce noise in the test file so I would keep it 👍
| await retrySpawnProcess(this.mavenConfig.mavenCliPath, [ | ||
| 'gpg:sign-and-deploy-file', | ||
| `-Dfile=${bomFile}`, | ||
| `-DpomFile=${bomFile}`, |
There was a problem hiding this comment.
don't need to set -Dtypes=jar,jar here?
There was a problem hiding this comment.
AbhiPrasad
left a comment
There was a problem hiding this comment.
Just wanted to get the comment about the zip behaviour address, and then I'll slam the ✅.
| * `undefined` if there isn't any. | ||
| */ | ||
| private async getBomFileInDist(distDir: string): Promise<string | undefined> { | ||
| const pomFilepath = join(distDir, POM_DEFAULT_FILENAME); |
There was a problem hiding this comment.
Should this be something like possibleBomFilePath?
There was a problem hiding this comment.
It could, but not sure whether it should. We don't know whether it's a BOM, but we do know it's a POM (a BOM is a type of POM). So I think making a reference to BOM makes it less accurate; besides, some logic below treats any xml file as a potential BOM, so that would be confusing.
There was a problem hiding this comment.
Got it, that reasoning sounds good to me.
| // There may be several files in the ZIP-ed artifact with the same name, | ||
| // where the BOM may be one of them (there may not be a BOM). Files may be | ||
| // renamed when extracting the ZIP, so the default name (`pom-default.xml`) | ||
| // may not match. It's assumed that any renaming keeps the same extension, | ||
| // so all files with the same extension are checked to identify the BOM. |
There was a problem hiding this comment.
Can we simplify this logic by enforcing users of the target to pass in the bom file name as a craft option? I know we already wrote all of this out, but I'm scared of the regex test failing to account for all scenarios.
There was a problem hiding this comment.
No. Although the end goal is to run the target in unattended mode, it currently is meant to run in attended mode. When files with same name exist in a ZIP, OSX (where this is going to run for now, at least until running in unattended mode is fully supported) asks for a name to rename the file. Force the user to set the same filename set in the config file is a clear no for me.
Regarding other scenarios, you're right this isn't solid enough. The Linux file system (where the Craft image will run), OSX, and running in attended mode is a lot of scenarios. The thing that should never change is the file extension (even Linux shouldn't modify it), and that's the reason behind it. The little prose in the PR description is ambiguous, but I left it like that on purpose. Once the maven target gets more work (to support unattended mode), these scenarios should be handled.
I'll add a TODO in the code for this, which clearly missing.
AbhiPrasad
left a comment
There was a problem hiding this comment.
Thanks for the additional context + TODO 🚀
| // where the BOM may be one of them (there may not be a BOM). Files may be | ||
| // renamed when extracting the ZIP, so the default name (`pom-default.xml`) |
There was a problem hiding this comment.
Files may be renamed when extracting the ZIP
How so? We control how we unzip, right?
| for (const f of potentialPoms) { | ||
| if (await this.isBomFile(f)) { | ||
| return f; | ||
| } | ||
| } |
There was a problem hiding this comment.
Strongly recommend using sync APIs for isBomFile and replacing this block with potentialPoms.find(f => this.isBomFile(f)) if the number of files is small and the file contents are small. Would be much faster and simpler code.
There was a problem hiding this comment.
Not sure how big the size of the contents can be, but went with the declarative way.
maven targetmaven target
Add support for BOM files in the
maventarget.This is based on previous Kotlin code, but that's incomplete. A BOM file is a POM file with a
<packaging>pom</packaging>key.An artifact (the
zipdownloaded from GHA) may contain a POM and a BOM, both with the same name (pom-default.xmlis the default name). In case both are present, unzipping the artifact may lead to having an unknown name for the BOM file (e.g. the POM is thepom-default.xml, and the BOM is ??). Thus, if the default file isn't a BOM, allxmlfiles in the artifact are checked to not miss the BOM. If no BOM is found, the distribution is uploaded as it is now (without this PR); and if a BOM is found, a different command to upload files is run. Once artifacts have been uploaded, the registry is closed.