Support for specifying dependencies from git url#100
Conversation
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
|
I have the setup where you can specify the nodejs sdk using a It's not clear to me what the right option is for now or if it's worth just avoiding this issue in a different way for the demo, so I'm just going to merge this PR as is without that change and then put my commit for the package.json git+ssh dependency support in the separate workflow PR here where we can figure it out: #95 |
aluzzardi
left a comment
There was a problem hiding this comment.
Argh, forgot to click submit yesterday. I just had a tiny nit that can be discarded
| return status.Errorf(codes.Internal, "no sshid in metadata") | ||
| } | ||
| id := v[0] | ||
| if id != daggerSockName { |
There was a problem hiding this comment.
tiny nit: why the reversal for this (e.g. happy path inside the if, error outside)?
Typically in idiomatic Go the "if" would be as small as possible.
https://about.sourcegraph.com/blog/go/idiomatic-go
- Handle unexpected cases and errors early and return often.
- Keep common or happy paths de-dented.
- When it’s not possible refactor and/or redesign.
There was a problem hiding this comment.
Oh I previously had other changes in this function where it made more sense to have this change. I undid them as I took a different approach but forget to undo the reversal here, will fix in my other open PR
Sounds good What if we implement our own "subdirectory of the git url"? Basically wrapping |
@aluzzardi currently went with the ssh auth sock approach because I think it will allow us to also seamlessly support specifying the nodejs sdk as a git url in
package.json: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#git-urls-as-dependenciesI'm guessing the token-based approach you mentioned could be made to work with that too, but not immediately sure so I just went with this for now. The downside of this approach is that it requires the user have an ssh agent (with
SSH_AUTH_SOCKenv), but that's probably okay for now (should unblock the use case in the demo if nothing else).I need to verify the
package.jsongit url will also work here, will update this with that before merging, but let me know if there's comments in the meantime.