-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat(pacmak): Add support for non-Maven Central repository #3893
Conversation
Add CLI arguments that will inject appropriate values into maven settings file so that non-Maven Central dependencies can be accessed at build time
@RomainMuller Do mind approving the workflow runs and possibly reviewing when you get a chance? |
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.
That looks pretty good... I have some suggestions for improvements, but some I'm not even sure myself whether they are actionable in reality...
It would also be nice if you could document usage in https://github.com/aws/jsii/blob/main/gh-pages/content/user-guides/lib-author/configuration/targets/java.md as well.
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.', | ||
); |
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.
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?
.option('maven-extra-repository-password', { | ||
type: 'string', | ||
desc: 'Password for authenticating to extra repository', | ||
defaultDescription: 'No extra repository is registered.', | ||
default: undefined, | ||
hidden: true, | ||
}) |
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.
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.
const repos = localRepos.map((repo) => ({ | ||
id: repo.replace(/[\\/:"<>|?*]/g, '$'), | ||
url: `file://${repo}`, | ||
})); |
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.
We could simplify this a bit I feel...
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 _
.
'Extra repository requested but no URL was provided! Use the --maven-extra-repository-url argument to provide a value.', | ||
); | ||
|
||
repos.push({ id: extraRepositoryId, url: extraRepositoryUrl }); |
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.
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?
Instead of exposing every Maven option via the CLI, I suggest that we provide a single option for users to override the setting file that is currently hard-coded by |
I agree that this might offer more flexibility... |
#4068 should help address this. With that change in place, I'm able to define my own repository settings in a configuration file and pass it to
|
Solved by #4068 |
Add CLI arguments that will inject appropriate values into maven settings file so that non-Maven Central dependencies can be accessed at build time
This was specifically written to work with AWS CodeArtifact per the setup guide, but other repositories will work as well.
Example usage:
Username/password can be omitted, but ID + URL are required for the maven settings to be updated.
Should also address issue #2404 / discussion #3465
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.