DRAFT - add support for private github URLs#531
DRAFT - add support for private github URLs#531bvaisvil wants to merge 3 commits intodnanexus:developfrom
Conversation
Ben-Habermeyer
left a comment
Ben-Habermeyer
left a comment
There was a problem hiding this comment.
looks pretty good to me, though I admit my scala knowledge is limited. I provided a pointer to their dev docs and have one question about mixed imports
| @@ -0,0 +1,3 @@ | |||
| java temurin-11.0.21+9 | |||
There was a problem hiding this comment.
nit: DX may ask to revert this however I personally appreciate it
|
|
||
| /** | ||
| * Determines if authentication should be used for the given URI. | ||
| * Only returns true if a token is configured AND the domain is in the allowed list. |
There was a problem hiding this comment.
checking my understanding here, if the import belongs to one of the allowed domains AND a token is set in the env then this protocol will be used. Is this done on an import-by-import basis?
What happens if we have imports from multiple sources? e.g.
import local.wdl
import http://raw.githubusercontent.com/remote/public.wdl
import https://github.com/company/repo/private.wdl
There was a problem hiding this comment.
If I'm reading this correctly, it does it per source in the resolver (above). Likely worth just trying it just to make sure but it looks like that's what it's doing.
| imports, | ||
| Vector(DxFileAccessProtocol()), | ||
| logger | ||
|
|
There was a problem hiding this comment.
I think this is right and the order look like it will be the same (imports, now http then dx file access)
Here's the code for LocalFileAccessProtocol and FileSourceResolver
https://github.com/dnanexus/dxScala/blob/8b056342eb52ebdb800b63d2c6179a4d2e6ed77f/common/src/main/scala/dx/util/FileSource.scala#L512
https://github.com/dnanexus/dxScala/blob/8b056342eb52ebdb800b63d2c6179a4d2e6ed77f/common/src/main/scala/dx/util/FileSource.scala#L736
3 participants
I'll write up a more detailed description ASAP. This draft is meant for myriad folks to sanity check before I ask anyone from DNANexus to review.
Please note that an LLM was used in the creation of this PR.