-
-
Notifications
You must be signed in to change notification settings - Fork 20
feat(maven): Support BOM files in maven target
#270
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
Changes from all commits
f049c79
69ef700
70ea6f0
2a431d4
1389773
cdc2303
f10bd39
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 |
|---|---|---|
| @@ -1,7 +1,12 @@ | ||
| import { homedir } from 'os'; | ||
| import { join } from 'path'; | ||
| import { NoneArtifactProvider } from '../../artifact_providers/none'; | ||
| import { MavenTarget, targetOptions, targetSecrets } from '../maven'; | ||
| import { | ||
| MavenTarget, | ||
| POM_DEFAULT_FILENAME, | ||
| targetOptions, | ||
| targetSecrets, | ||
| } from '../maven'; | ||
| import { retrySpawnProcess } from '../../utils/async'; | ||
| import { withTempDir } from '../../utils/files'; | ||
|
|
||
|
|
@@ -11,6 +16,11 @@ jest.mock('fs', () => ({ | |
| ...jest.requireActual('fs'), | ||
| promises: { | ||
| writeFile: jest.fn(() => Promise.resolve()), | ||
| readFile: jest.fn((file: string) => file), | ||
| readdir: async () => Promise.resolve([]), // empty dir | ||
| access: jest.fn().mockImplementation(() => { | ||
| // do nothing | ||
| }), | ||
| }, | ||
| })); | ||
|
|
||
|
|
@@ -109,10 +119,8 @@ describe('publish', () => { | |
| async () => void callOrder.push('closeAndRelease') | ||
| ); | ||
|
|
||
| const version = '1.0.0'; | ||
| const revision = 'r3v1s10n'; | ||
|
|
||
| await mvnTarget.publish(version, revision); | ||
| await mvnTarget.publish('1.0.0', revision); | ||
|
Contributor
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. Why change to directly passing in the string vs. a variable like before?
Contributor
Author
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. The version isn't being used anywhere else, so IMO removes a bit of noise (like in others
Contributor
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. No I have no strong opinions, just wondering. I agree it helps reduce noise in the test file so I would keep it 👍 |
||
| expect(gradlePropsMock).toHaveBeenCalledTimes(1); | ||
| expect(uploadMock).toHaveBeenCalledTimes(1); | ||
| expect(uploadMock).toHaveBeenLastCalledWith(revision); | ||
|
|
@@ -127,7 +135,7 @@ describe('publish', () => { | |
| ]); | ||
| }); | ||
|
|
||
| test('upload', async () => { | ||
| test('upload POM', async () => { | ||
| // simple mock to always use the same temporary directory, | ||
| // instead of creating a new one | ||
| (withTempDir as jest.MockedFunction<typeof withTempDir>).mockImplementation( | ||
|
|
@@ -143,9 +151,11 @@ describe('publish', () => { | |
| mvnTarget.artifactProvider.downloadArtifact = jest | ||
| .fn() | ||
| .mockResolvedValueOnce('artifact/download/path'); | ||
| mvnTarget.isBomFile = jest.fn().mockResolvedValueOnce(false); | ||
|
|
||
| await mvnTarget.upload('r3v1s10n'); | ||
| expect(retrySpawnProcess).toBeCalledTimes(1); | ||
|
|
||
| expect(retrySpawnProcess).toHaveBeenCalledTimes(1); | ||
| const callArgs = (retrySpawnProcess as jest.MockedFunction< | ||
| typeof retrySpawnProcess | ||
| >).mock.calls[0]; | ||
|
|
@@ -172,6 +182,49 @@ describe('publish', () => { | |
| expect(cmdArgs[8]).toBe('--settings'); | ||
| expect(cmdArgs[9]).toBe(DEFAULT_OPTION_VALUE); | ||
| }); | ||
|
|
||
| test('upload BOM', async () => { | ||
| // simple mock to always use the same temporary directory, | ||
| // instead of creating a new one | ||
| (withTempDir as jest.MockedFunction<typeof withTempDir>).mockImplementation( | ||
| async cb => { | ||
| return await cb(tmpDirName); | ||
| } | ||
| ); | ||
|
|
||
| const mvnTarget = createMavenTarget(); | ||
| mvnTarget.getArtifactsForRevision = jest | ||
| .fn() | ||
| .mockResolvedValueOnce([{ filename: 'mockArtifact.zip' }]); | ||
| mvnTarget.artifactProvider.downloadArtifact = jest | ||
| .fn() | ||
| .mockResolvedValueOnce('artifact/download/path'); | ||
| mvnTarget.isBomFile = jest.fn().mockResolvedValueOnce('path/to/bomfile'); | ||
|
|
||
| await mvnTarget.upload('r3v1s10n'); | ||
|
|
||
| expect(retrySpawnProcess).toHaveBeenCalledTimes(1); | ||
| const callArgs = (retrySpawnProcess as jest.MockedFunction< | ||
| typeof retrySpawnProcess | ||
| >).mock.calls[0]; | ||
|
|
||
| expect(callArgs).toHaveLength(2); | ||
| expect(callArgs[0]).toEqual(DEFAULT_OPTION_VALUE); | ||
|
|
||
| const cmdArgs = callArgs[1] as string[]; | ||
| expect(cmdArgs).toHaveLength(7); | ||
| expect(cmdArgs[0]).toBe('gpg:sign-and-deploy-file'); | ||
| expect(cmdArgs[1]).toMatch( | ||
| new RegExp(`-Dfile=${tmpDirName}.+${POM_DEFAULT_FILENAME}`) | ||
| ); | ||
| expect(cmdArgs[2]).toMatch( | ||
| new RegExp(`-DpomFile=${tmpDirName}.*${POM_DEFAULT_FILENAME}`) | ||
| ); | ||
| expect(cmdArgs[3]).toBe(`-DrepositoryId=${DEFAULT_OPTION_VALUE}`); | ||
| expect(cmdArgs[4]).toBe(`-Durl=${DEFAULT_OPTION_VALUE}`); | ||
| expect(cmdArgs[5]).toBe('--settings'); | ||
| expect(cmdArgs[6]).toBe(DEFAULT_OPTION_VALUE); | ||
| }); | ||
| }); | ||
|
|
||
| describe('get gradle home directory', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,14 +5,14 @@ import { | |
| } from '../artifact_providers/base'; | ||
| import { BaseTarget } from './base'; | ||
| import { homedir } from 'os'; | ||
| import { basename, join, parse } from 'path'; | ||
| import { promises as fsPromises } from 'fs'; | ||
| import { basename, extname, join, parse } from 'path'; | ||
| import { constants as fsConstants, promises as fsPromises } from 'fs'; | ||
| import { checkExecutableIsPresent, extractZipArchive } from '../utils/system'; | ||
| import { retrySpawnProcess } from '../utils/async'; | ||
| import { withTempDir } from '../utils/files'; | ||
| import { checkEnvForPrerequisite } from '../utils/env'; | ||
| import { ConfigurationError } from '../utils/errors'; | ||
| import { stringToRegexp } from '../utils/filters'; | ||
| import { checkEnvForPrerequisite } from '../utils/env'; | ||
|
|
||
| const GRADLE_PROPERTIES_FILENAME = 'gradle.properties'; | ||
|
|
||
|
|
@@ -21,6 +21,9 @@ const GRADLE_PROPERTIES_FILENAME = 'gradle.properties'; | |
| * https://docs.gradle.org/current/userguide/build_environment.html#sec:gradle_environment_variables | ||
| */ | ||
| const DEFAULT_GRADLE_USER_HOME = join(homedir(), '.gradle'); | ||
| export const POM_DEFAULT_FILENAME = 'pom-default.xml'; | ||
| const POM_FILE_EXT = '.xml'; // Must include the leading `.` | ||
| const BOM_FILE_KEY_REGEXP = stringToRegexp('/<packaging>pom</packaging>/'); | ||
|
iker-barriocanal marked this conversation as resolved.
|
||
|
|
||
| export const targetSecrets = ['OSSRH_USERNAME', 'OSSRH_PASSWORD'] as const; | ||
| type SecretsType = typeof targetSecrets[number]; | ||
|
|
@@ -258,12 +261,95 @@ export class MavenTarget extends BaseTarget { | |
| * @param distDir directory of the distribution. | ||
| */ | ||
| private async uploadDistribution(distDir: string): Promise<void> { | ||
| const bomFile = await this.getBomFileInDist(distDir); | ||
| if (bomFile) { | ||
| this.logger.debug('Found BOM: ', bomFile); | ||
| await this.uploadBomDistribution(bomFile); | ||
| } else { | ||
| this.logger.debug('Did not find a BOM.'); | ||
| await this.uploadPomDistribution(distDir); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the path to the BOM file in the given distribution directory, and | ||
| * `undefined` if there isn't any. | ||
| */ | ||
| private async getBomFileInDist(distDir: string): Promise<string | undefined> { | ||
| const pomFilepath = join(distDir, POM_DEFAULT_FILENAME); | ||
|
Contributor
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. Should this be something like
Contributor
Author
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. 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
Contributor
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. Got it, that reasoning sounds good to me. |
||
| if (await this.isBomFile(pomFilepath)) { | ||
| return pomFilepath; | ||
| } | ||
|
|
||
| // 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`) | ||
|
Comment on lines
+285
to
+286
Member
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.
How so? We control how we unzip, right? |
||
| // 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. | ||
|
Comment on lines
+284
to
+288
Contributor
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. 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.
Contributor
Author
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. 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 I'll add a |
||
| // TODO: make sure all scenarios are considered and tested. | ||
| // Each file system may handle this case differently, and attended vs | ||
| // unattended mode also have different behaviours. It's not desired to get | ||
| // the BOM renamed in such a way that isn't handled by the target. | ||
| const filesInDir = await fsPromises.readdir(distDir); | ||
| const potentialPoms = filesInDir | ||
| .filter(f => extname(f) === POM_FILE_EXT) | ||
| .filter(f => f !== POM_DEFAULT_FILENAME) | ||
|
iker-barriocanal marked this conversation as resolved.
|
||
| .map(f => join(distDir, f)); | ||
|
|
||
| for (const f of potentialPoms) { | ||
| if (await this.isBomFile(f)) { | ||
| return f; | ||
| } | ||
| } | ||
|
Comment on lines
+299
to
+303
Member
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. Strongly recommend using sync APIs for
Contributor
Author
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. Not sure how big the size of the contents can be, but went with the declarative way. |
||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether the given POM is a BOM. | ||
| * | ||
| * A BOM file is a POM file with the following key: | ||
| * `<packaging>pom</packaging>`, usually named as `pom-default.xml`. | ||
| * | ||
| * @param pomFilepath path to the POM. | ||
| * @returns true if the POM is a BOM. | ||
| */ | ||
| public async isBomFile(pomFilepath: string): Promise<boolean> { | ||
| try { | ||
| await fsPromises.access(pomFilepath, fsConstants.R_OK); | ||
|
iker-barriocanal marked this conversation as resolved.
|
||
| const fileContents = await fsPromises.readFile(pomFilepath, { | ||
| encoding: 'utf8', | ||
| }); | ||
| return BOM_FILE_KEY_REGEXP.test(fileContents); | ||
| } catch (error) { | ||
| this.logger.warn( | ||
| `Could not determine if path corresponds to a BOM file: ${pomFilepath}\n` + | ||
| 'Error:\n', | ||
| error | ||
| ); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private async uploadBomDistribution(bomFile: string): Promise<void> { | ||
| await retrySpawnProcess(this.mavenConfig.mavenCliPath, [ | ||
| 'gpg:sign-and-deploy-file', | ||
| `-Dfile=${bomFile}`, | ||
| `-DpomFile=${bomFile}`, | ||
|
Contributor
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. don't need to set
Contributor
Author
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. |
||
| `-DrepositoryId=${this.mavenConfig.mavenRepoId}`, | ||
| `-Durl=${this.mavenConfig.mavenRepoUrl}`, | ||
| '--settings', | ||
| this.mavenConfig.mavenSettingsPath, | ||
| ]); | ||
| } | ||
|
|
||
| private async uploadPomDistribution(distDir: string): Promise<void> { | ||
| const { | ||
| targetFile, | ||
| javadocFile, | ||
| sourcesFile, | ||
| pomFile, | ||
| } = this.getFilesForMavenCli(distDir); | ||
| } = this.getFilesForMavenPomDist(distDir); | ||
|
|
||
| // Maven central is very flaky, so retrying with an exponential delay in | ||
| // in case it fails. | ||
|
|
@@ -288,7 +374,7 @@ export class MavenTarget extends BaseTarget { | |
| * @param distDir directory of the distribution. | ||
| * @returns record of required files. | ||
| */ | ||
| private getFilesForMavenCli(distDir: string): Record<string, string> { | ||
| private getFilesForMavenPomDist(distDir: string): Record<string, string> { | ||
| const moduleName = parse(distDir).base; | ||
| return { | ||
| targetFile: join(distDir, this.getTargetFilename(distDir)), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.