DRAFT - remove restriction on github only dagger modules

[purpleclay]

closes #6890

[purpleclay]

From initial testing, I can successfully use dagger modules hosted on other VCS platforms like GitLab. However, I have broken ModuleSourceKindLocal. I have left this change here for discussion purposes.

Some examples, with debug output attached:

./bin/dagger call --debug -m gitlab.com/purpleclay/daggerverse/[email protected] --src . mod-version
1.21.6

./bin/dagger call --debug -m gitlab.com/testing5740411/another/another2/another3/daggerverse/[email protected] --src . mod-version
1.21.6

nested-gitlab-groups.txt

Looking at the current logic and how this function is used, I feel there must be a better way to detect a local path provided to the -m flag on the CLI. I completely understand I lack knowledge of how the Dagger engine works. And there might be an easy way of doing this, which isn't that obvious to me.

[kpenfound]

I suspect currently if you had a local module in ./github.com/foo/bar and called -m github.com/foo/bar that would probably break for the same reason. Maybe local modules should always be prefixed by ./ or absolute path?

[purpleclay]

I was toying with using a convention like / or ./ to denote that the module is local, but omitting those is also a valid way of indicating a file path. Of course, it would be much better than making a remote module identifiable through a convention such as https://. The typing of those extra characters feels like a bad developer experience.

Could a distinction be made through the CLI itself? For example, could introducing a new flag for a local module be done? Of course, that would be a breaking change to the CLI.

I am happy to make the change you suggested for local module testing with the abovementioned prefixes to see how it looks and behaves.

[kpenfound]

I agree requiring https:// doesn't feel great. I think the lookup needs to happen both from the CLI directly and also during runtime for loading dependencies, so a different flag may not work anyway. Any ideas @jedevc @vito @sipsma @shykes ?

[jedevc]

cc @vito @kpenfound, I know there's a few internal discussion around this 🎉

[vito]

For continuity, the last time I looked into this I put together a proposal for "v2 refs".

It's a really tricky topic, and also ties in to potential product-level decisions like short-refs to modules in the Daggerverse - so just to manage expectations, I'm a little skeptical that we'll be able to resolve this quickly in a single PR, but I'm very happy that someone else is trying to move the needle here. 🙂

The sneaky hard part is that we need to distinguish the repo root (github.com/vito/daggerverse) from the sub-path (/apko). We know how to do this for github.com because we can just assume the first two path segments are user/repo and the rest is the sub-path. But to support truly arbitrary URLs we can't make that assumption. GitLab for example allows you to organize repositories into arbitrarily nested group hierarchies, so gitlab.com/foo/bar/baz/buzz could actually be ./buzz under the gitlab.com/foo/bar/baz repo. Go gets away with this because each code forge (GitHub, GitLab, etc.) returns special metadata in the response that tells it where the repo URL ends and the package path begins, along with tons of other unsightly tricks (see Why not just copy Go?). This complexity is what led me to v2 refs proposal.

I'm not totally sure where we go from here, but I'm glad it's active again, since I know we don't want to be married to github.com forever. I guess as a starting point, have you run into that tricky part yet, and do you have any thoughts on the v2 refs proposal?

Thanks!

[purpleclay]

@vito Thanks for the link to the v2 refs proposal. It shed some light on the thought process for a proposed evolution to referencing remote and local Dagger modules. It has given me insight into how Dagger resolves tags and dependencies.

I agree that this should be resolved in a series of smaller PRs, introducing functionality in tiny chunks to gauge its impact.

I have some thoughts and questions (please be mindful that I haven't trawled through the entire code base 😄):

1. The proposed URI scheme is well thought out and provides a solid way to differentiate a Dagger module's location. However, users will undoubtedly not follow it to the letter, so a hybrid approach to how Go handles VCS detection could be worthwhile (especially in the edge cases where well-known offerings such as github.com and gitlab.com are used).
2. I agree with the conventions for detecting a local module (., .., ./foo, ../foo, and /foo). However, accepting foo can make differentiating between a local and remote module hard if foo/sub1/sub2 was provided. Using file:// would allow a solid distinction, but I imagine users won't want to type those extra characters.
   • Could the distinction between a local and remote module not be made within the dagger CLI as soon as the -m flag is provided? A quick os.Stat() would give you that answer. In hindsight, the location of that check was entirely in the wrong place within my PR.
   • Regarding dependencies, the Dagger CLI (dagger install) controls the convention for a local dagger module. It should enforce the use of the expected conventions (., .., ./foo, and ../foo). Again, it feels like a simple os.Stat() would suffice to detect a local dependency (as relative paths only make sense here).

My naive solution allowed me to work with GitLab nested groups and was designed to promote discussion and increase my understanding of how Dagger resolves the identified Git ref.

[vito]

@purpleclay re: detecting local references, yeah I agree that seems worth a quick os.Stat check. I suppose the follow-up decision is what happens when you say github.com/foo/bar and you have ./github.com locally. Basically, do we always os.Stat the first element, or only apply it to word-like path elements? Doesn't feel like there's a right answer there.

Also: what if I say foo/bar/baz but foo is actually a hostname on my LAN? Do we os.Stat foo, not find it, and assume it's a repo URL? 🤷‍♂️

Worth noting that we also now support having foo refer to a named module dependency, which wasn't a thing back when the original proposal was written. Seems solveable though. I guess module names could take highest precedence?

re: the proposal in general, we never reached consensus on it, so I wouldn't hold it up as the golden standard, but I'm glad it made sense to you too. :P I think the most controversial parts are a) the various URL shorthands and b) the use of // as a generic repo vs. path separator.

---

@shykes Have you had any new thoughts re: shorthand for Daggerverse modules?

[purpleclay]

@vito Can I ask how a named module is resolved?

[vito]

@purpleclay Sure thing - so in that case the CLI will look upward from the current directory until it finds the nearest dagger.json, and then it'll check each of the named modules from there:

daggerverse on  main [!?] via 🐹 
❯ cat docker/dagger.json 
{
  "name": "docker",
  "sdk": "go",
  "dependencies": [
    {
      "name": "proxy",
      "source": "github.com/kpenfound/dagger-modules/proxy@053eeb803f87005cf04ae3ad6cc4bde6031b4d35"
    }
  ],
  "source": "."
}

daggerverse on  main [!?] via 🐹 
❯ cd docker

daggerverse/docker on  main [!?] via 🐹 
❯ dagger-dev -m proxy functions
Name           Description
ctr            An OCI-compatible container, also known as a Docker container.
service        Get the proxy Service
with-service   Add a service to proxy

[github-actions]

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This PR was closed because it has been stalled for 7 days with no activity.

[vito]

@purpleclay Letting the stale bot close this one out seems OK - conversation is active in #7218 now and @grouville is actively working on this. Thanks for getting the ball rolling! 🙏