Skip to content

Alternative fix for https://github.com/polynote/polynote/issues/1009#1019

Open
jelmerk wants to merge 1 commit intopolynote:masterfrom
jelmerk:issue1009-alternative
Open

Alternative fix for https://github.com/polynote/polynote/issues/1009#1019
jelmerk wants to merge 1 commit intopolynote:masterfrom
jelmerk:issue1009-alternative

Conversation

@jelmerk
Copy link

@jelmerk jelmerk commented Jan 3, 2021

See discussion in : #1011

@jeremyrsmith
Copy link
Contributor

I like this solution! It uses the local JAR for dependencies that come from a credentialed host; otherwise uses the direct URL.

Copy link
Contributor

@jeremyrsmith jeremyrsmith left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks; let me know if you need help with them! Thanks for coming up with this alternate solution; I like how minimal it is!

val passwordProtectedHosts = directCredentials.map(_.host).toSet

val jars = deps.map { case (url, file) =>
if (passwordProtectedHosts.contains(URI.create(url).getHost)) file.getAbsolutePath
Copy link
Contributor

Choose a reason for hiding this comment

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

URI.create could throw if the URI string isn't correctly formatted, so that should be inside an IO. Also, file.getAbsolutePath will do blocking IO, so it should be inside effectBlocking.


private def loadCredentials(credentials: Credentials): URIO[Logging, List[DirectCredentials]] = credentials.coursier match {
case Some(Credentials.Coursier(path)) =>
Task(CoursierCredentials(new File(path), optional = false).get().toList)
Copy link
Contributor

Choose a reason for hiding this comment

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

Credentials#get() from coursier does blocking IO, so it should be in effectBlocking() rather than Task() (so this would have to return URIO[Blocking with Logging, List[DirectCredentials] – I'd just make it URIO[BaseEnv, List[DirectCredentials] which includes both of those)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants