Skip to content

feat(pacmak): Add support for non-Maven Central repository #3893

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

Closed
icj217 wants to merge 3 commits into from
Closed

feat(pacmak): Add support for non-Maven Central repository #3893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c2e81cc
feat(pacmak): Add support for extra maven repository
Dec 30, 2022
7f57d8c
Merge branch 'main' into add_extra_mvn_repo
RomainMuller Feb 16, 2023
01685a7
Merge branch 'main' into add_extra_mvn_repo
RomainMuller Feb 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions packages/jsii-pacmak/bin/jsii-pacmak.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,34 @@ import { VERSION_DESC } from '../lib/version';
default: undefined,
hidden: true,
})
.option('maven-extra-repository-id', {
type: 'string',
desc: 'ID of additional repository to add to Maven settings file',
defaultDescription: 'No extra repository is registered.',
default: undefined,
hidden: true,
})
.option('maven-extra-repository-url', {
type: 'string',
desc: 'URL of additional repository to add to Maven settings file',
defaultDescription: 'No extra repository is registered.',
default: undefined,
hidden: true,
})
.option('maven-extra-repository-username', {
type: 'string',
desc: 'Username for authenticating to extra repository',
defaultDescription: 'No extra repository is registered.',
default: undefined,
hidden: true,
})
.option('maven-extra-repository-password', {
type: 'string',
desc: 'Password for authenticating to extra repository',
defaultDescription: 'No extra repository is registered.',
default: undefined,
hidden: true,
})
Comment on lines +177 to +183
Copy link
Contributor

@RomainMuller RomainMuller Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing passwords over CLI arguments is notoriously risky... There's many avenues through which program arguments may be visible by other parties...

I get that these may be "short-lived" credentials here (so less risky to maybe leak somewhere), but I'd still rather have an alternate (safer?) way to provide the password... Not sure what the typical maven practice there is...

This ultimately makes me wonder if we shouldn't instead allow one to provide a "base" XML maven settings file to use, which can include some repos, credentials, etc... This is just an idea, though... I don't know that Maven settings XML documents allow "extending" others without actually copying + modifying them... Which sounds like a lot of unneeded work.

.option('validate-assemblies', {
type: 'boolean',
desc: 'Whether jsii assemblies should be validated. This can be expensive and is skipped by default.',
Expand Down
43 changes: 39 additions & 4 deletions packages/jsii-pacmak/lib/targets/java.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,43 @@ export class JavaBuilder implements TargetBuilder {

const profileName = 'local-jsii-modules';
const localRepository = this.options.arguments['maven-local-repository'];

const repos = localRepos.map((repo) => ({
id: repo.replace(/[\\/:"<>|?*]/g, '$'),
url: `file://${repo}`,
}));
Comment on lines +289 to +292
Copy link
Contributor

@RomainMuller RomainMuller Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could simplify this a bit I feel...

Suggested change
const repos = localRepos.map((repo) => ({
id: repo.replace(/[\\/:"<>|?*]/g, '$'),
url: `file://${repo}`,
}));
const repos = localRepos.map((repo, idx) => ({
id: `_local_${idx}`,
url: `file://${repo}`,
}));

If doing this, we can ensure "no collisions" by documenting & encoding that "extra" repo IDs may not have a name starting with _.


let servers = undefined;

// Add repository provided by user to support edge cases
// e.g. corporate proxies and other remote repositories
const extraRepositoryId: string | undefined =
this.options.arguments['maven-extra-repository-id'];
const extraRepositoryUrl: string | undefined =
this.options.arguments['maven-extra-repository-url'];
const extraRepositoryUsername: string | undefined =
this.options.arguments['maven-extra-repository-username'];
const extraRepositoryPassword: string | undefined =
this.options.arguments['maven-extra-repository-password'];

if (extraRepositoryId) {
if (!extraRepositoryUrl)
throw new Error(
'Extra repository requested but no URL was provided! Use the --maven-extra-repository-url argument to provide a value.',
);
Comment on lines +307 to +311
Copy link
Contributor

@RomainMuller RomainMuller Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this maybe be made a single option? Something like --maven-extra-repository=id=url or if you need credentials (password caveat from above still applies) --maven-extra-repository=id=username:password@url (although maybe yargs won't like this format?).

Then - is there a use-case for having more than 1 extra repository configured?


repos.push({ id: extraRepositoryId, url: extraRepositoryUrl });
Copy link
Contributor

@RomainMuller RomainMuller Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest turning repos into a Map<string, string> (key is the ID, value is the URL), so it's easier to guarantee no 2 repositories have the same ID?

servers = {
server: [
{
id: extraRepositoryId,
username: extraRepositoryUsername,
password: extraRepositoryPassword,
},
],
};
}

const settings = xmlbuilder
.create(
{
Expand All @@ -302,15 +339,13 @@ export class JavaBuilder implements TargetBuilder {
localRepository: localRepository
? path.resolve(process.cwd(), localRepository)
: path.resolve(where, '.m2', 'repository'),
servers,
// Register locations of locally-sourced dependencies
profiles: {
profile: {
id: profileName,
repositories: {
repository: localRepos.map((repo) => ({
id: repo.replace(/[\\/:"<>|?*]/g, '$'),
url: `file://${repo}`,
})),
repository: repos,
},
},
},
Expand Down