Support for maven servers requiring authentication #215
Conversation
|
Bump :) |
There was a problem hiding this comment.
Sorry for the latency.
Can we please split this into two PRs to make it easier for us to understand the change history?
Secondly, I will merge this PR, but I have a lot of anxiety about having two backends each with their own feature sets. We have seen bugs with aether and no longer use it at Stripe in favor of coursier, which is also much faster. It would be really nice to see if we can support everything in one backend.
| private val m2Home = new File(new File(System.getProperty("user.home")), "/.m2/") | ||
| private val m2SecurityXml = new File(m2Home, "settings-security.xml") | ||
|
|
||
| private val settingsDecrypter: SettingsDecrypter = { |
There was a problem hiding this comment.
this seems like a separate PR, can we please separate from the warning/error on imports?
| logger.warn (s"unsupported import-scope dependency: $d") | ||
| false | ||
| case ImportScopeHandling.Error => | ||
| sys.error (s"unsupported import-scope dependency: $d") |
There was a problem hiding this comment.
do you have any idea how much work it is to support this? I have not looked in detail, but I think it also coming up in #220
|
sorry for the latency |
Without this, Couriser will fail if any transitive dependencies depend on a POM.
This ensures that all errors are visible to the user, rather than just the first.
After digging into it a bit, I believe I've gotten authentication working for the Coursier backend as well. Given that it's both more reliable and faster, I'm happy to drop the |
rdnetto4
force-pushed
the
adding-server-auth
branch
from
1c390b0
to
ab35711
Compare
rdnetto4
changed the title
| import org.slf4j.LoggerFactory | ||
| import scala.collection.JavaConversions._ | ||
|
|
||
| class SettingsLoader { |
There was a problem hiding this comment.
Yes, that definitely makes more sense.
| servers.map { case MavenServer(id, t, u) => | ||
| var server = settings.getServer(id) |
There was a problem hiding this comment.
Can we use a val here and Option(settings.getServer(id)).getOrElse(new Server)
The tricky part of this what we test. In the resolvers, we're simply passing the credentials through to aether/coursier, and I'm not sure there's much value in testing that. Similarly, The most likely cause of a regression in this functionality would be forgetting to pass the credentials through, like here, but that feels like the sort of problem best caught by an integration test that actually tries to fetch an artifact requiring authentication... |
|
I’ve merged. It would be good to see if we can add an integration test to prevent future breaks. Thanks for the PR! |
|
I'm late to the game, but finally gave this a whirl instead of my own in-house hackery to cope with our authed repos. Resolution worked without a hitch, getting creds from my What secret am I missing to getting that second part working, or is that still an issue? (FWIW, my local hack I'm using currently is to identify the authenticated servers and have them use bazel-tools' |
|
@deadmoose You can't. Bazel |
This PR contains three main changes:
It adds support for maven servers which require authentication in AetherResolver. (I haven't been able to figure out the correct place in CoursierResolver to hook this in - it looks like it needs to be done on the creation of every
Artifact)The second change is to improve the handling of import-scoped dependencies, so that users can opt-in to ignoring them instead of failing the conversion. This makes sense for a large project we're converting to Bazel, as all the usages of import-scoped dependencies in our transitive closure would be overridden by our direct dependencies anyway.
The third change is to compute the SHA256 checksum on the AetherResolver codepath, as this was previously not done and is relied upon by the generated
workspace.bzl.