Skip to content

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

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

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

Conversation

@jelmerk

@jelmerk jelmerk commented Jan 3, 2021

Copy link
Copy Markdown

See discussion in : #1011

@jeremyrsmith

Copy link
Copy Markdown
Contributor

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

@jeremyrsmith jeremyrsmith left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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