-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow calling core functions from the CLI #6947
Comments
FYI @sipsma, this may be a duplicate, after a quick search I couldn't find a direct equivalent, but may not have found it. |
Would you use
|
Good question... any thoughts @sipsma @vito @aluzzardi @jedevc ? |
Good point - I like the idea of needing to explicitly select it, so yes, we'd probably need to reserve this ASAP. |
Reserving |
Would it break |
Actually it doesn't. Just need to choose a different name if installing so that you can't use Basically, only the exact |
Wdyt of |
I think the downside of both options, is that the module name doesn't map to the codegen namespace.
I know that you don't actually have to install core, but conceptually it is preinstalled. So the mismatch is potentially confusing. Also if we ever want to support multiple |
Let’s see, the main difference when loading core vs a module named “core” is this:
Just brain dumping here to make a point… The default for
Those would avoid a naming conflict, but are they intuitive? The thing is, I don’t think anyone is going to “guess” to use End brain dump As for using multiple Just trying to see if there’s value in trying to keep this as a possibility. But to be clear, I don’t think supporting calling core functions hurts this ability. |
Throwing another option into the mix: Might be a little odd but it'd at least be leaning on pre-existing GraphQL terminology that we already have to reckon with, as opposed to "core" which makes sense but comes with the cost of claiming another word. Maybe that could even work with multiple dagger -m github.com/vito/daggerverse/apko -m query call apko --help
# equivalent to:
dagger -m github.com/vito/daggerverse/apko call --help Though it's not obvious what utility that has, since it's not like modules can monkey-patch anymore. |
What about |
I thought about that but we need something to avoid naming conflicts like |
What's the naming conflict we need to avoid? |
Just disambiguity. If you have a Just trying to avoid having to reserve a name if there's something we can use to disambiguate. |
The output in #6947 (comment) has been reduced because of So all that remains is to finish bikeshedding on what value to use for The You can use |
I prefer cc @sipsma |
I see too problems though:
Footnotes
|
That's true, but not a dealbreaker IMO. We should assume there will be more "special" modules in the future (ie. a stdlib) so we will probably need a way to list those modules in the future anyway.
This problem isn't limited to the CLI though, is it? For example, what happens if I install a module named "container" today, and then try to call TLDR: yes name conflicts with core modules are a problem, but I think a problem we already have and should solve separately. |
I put in a footnote that you wouldn't be able to create a module with that name but you can:
Both cases allow you to use this today: ❯ dagger -m container functions
Name Description
container-echo Returns a container that echoes whatever string argument is provided
grep-dir Returns lines that match a pattern in the files of the provided Directory What's better?
I prefer checking core first before falling back to normal module loading. It's more performant, but potentially confusing. However I don't think anyone's going to actually be in this situation. It's possible but unlikely. |
|
If the module with the dependency has sources, it'll fail to |
In that case, I think it's fine to also fail in the same situation in the CLI. Consistency is probably better. For the subdirectory case, I still think core should win (because it's easy to disambiguate), but if it's easier to do it another way, I can live with that too. TLDR: I think both edge cases can be handled, even if I'm not 100% sure how. And I think the better UX of WDYT? |
I'll just look for a match under As for the UX, I'll just miss the list of core functions, but we do need a separate feature to list modules (DEV-3629), for example to list installed modules that you can run directly. There's also no way to display the module's description today so this could be it. In terms of consistency, doing Even though a user module adds a field to the root type, we've been talking about the core module as being the group of all other functions under "Query". Does it make sense that Some core modules don't return an object with functions, which violates the assumption that a module's constructor returns the module's main object type. In the case of There's also the I don't think these are deal breakers, I'm just doing a barometer check on the terminology. If everything is a module, it's natural that some things in the core API will feel like fitting a square peg in a round hole because it wasn't a design we had from the start. |
Personally, I kind of think core as a module, and Here's a few concrete reasons I think of it that way:
To me, it feels like there's a lot to unpack around how multiple core modules would work - and I think if we were to do it, it would ideally require a bit of restructuring of the APIs themselves to make this feel planned + neat (which is disruptive). I think being able to call core APIs from the CLI is more important to be able to do sooner-rather-than-later. Is there anything stopping us from having a In regards to where we attach the methods - I could imagine a future deprecation where we move all of our core module methods to |
Not technically.
Yeah, that was my thinking too but the current design was growing on me. Let me lash this out and make a summary of the options. RationaleIt’s the core API itself that doesn’t map all that well to the concept of modules. The current state of the core module was introduced in a Groundwork for exposing core API in CLI PR, but it doesn’t mean that design is finalized. Especially with discussions of a possible stdlib module. So if we do end up grouping it all under Another way to solve the mismatch is to decouple module loading with constructor selection. The - dagger --mod=parrot call parrot echo --msg=hello
+ dagger --mod=parrot call echo --msg=hello But now we have a mismatch with core functions. We can solve that by decoupling module loading with constructor selection, using perhaps a dagger --mod=parrot call --dag=parrot echo --msg=hello This way, if the default for dagger --mod=parrot call echo --msg=hello But would allow you to specify a core function instead (note that this way, "container" doesn’t need to be a module): dagger --mod=parrot call --dag=container from --address=alpine Granted, in this case it’s a waste to load the user module so you’ll likely want one or the other: dagger call --dag=version However, since dagger --mod="" call --dag=version But… at this point, if every time you need On one hand, the CLI could clear dagger --mod="" call version So
Which still feels consistent to me. It’s at this point that I tried finding something a little more ergonomic and ended up liking this one better: dagger -m- call version Now if we turn core functions into modules, but keep putting dagger -m version call More of a stretch to explain. Bikeshedding options
After re-considering those options, I’m leaning more towards Using |
Out of all the options, I'm happy with any of I would rather not have I would rather not have |
Also good with It's a little weird that the flag reads like "ok load a module, no wait, load NO modules!" - but it's also a low-risk iterative step, and maybe another iteration driven by external factors will make this all clearer. |
I think we should focus on options that embrace the technical reality of the core API: that is, that it is core. These options include:
Arguing for dedicated core commands
Core functions are different, they are baked in - maybe we don't need to jump through the hoops in the first place?
|
(jumping in here way too late, apologies!) I originally liked
I think this is true right now. I spent a while typing out what we could do to close this gap though and almost all of it is solvable. However, the one part I can't figure out is the fact that the core API is "flattened" (
I think this might be my favorite option at the moment. There is a downside in that we now need to deal with conflicts between top-level core APIs and top-level
An intriguing side-effect of this is that it almost makes the entire CLI programmable as just the dagger API.
Thinking out loud some more, it's also almost tempting at that point to support:
This is an extremely raw thought, would not be surprised if there's corner cases, but posting anyways so I don't delay commenting for the sake of thinking any longer 😅 |
Hm, if we do this, we already have a conflict - To be honest, I quite liked when we switched away from all the previous "verbs" of the old cli, and moved to everything under
I think I disagree - I think If we split these functionalities out, note that now you can interact with the core API in a couple ways:
The call syntax is now kind of "everywhere" - if a user is looking at a cli subcommand, does that command take this call syntax or not? Not all cli subcommands can be expressed in a core api call - like |
This particular conflict is fine I think. We are not obligated to map everything 1-1.
I think that's a good thing. The API was carefully crafted to be human-readable, and to make sense when explored from, say, a GraphQL playground. It's fair to say "if it's in the core API it should be in the CLI, and vice versa".
No question, But Separately from this, I think it's inevitable that we'll need
Right but "kind of" is the whole problem.
I like this because it emphasizes that one is an external module, and the other is builtin.
Here's a possible alternative, for preventing "proliferation" of the call syntax everywhere in the CLI: dagger call --core FUNC: call a core function
dagger call --module MOD FUNC: call a function from a module
dagger core FUNC: call a core function (convenience) Examples: dagger core \
container \
from --address=alpine \
with-directory --source=$(dagger core directory with-new-file --path=hello.txt --contents='hello world!') \
terminal Would be equivalent to: dagger call --core \
container \
from --address=alpine \
with-directory --source=$(dagger call --core directory with-new-file --path=hello.txt --contents='hello world!') \
terminal |
As I mentioned in my last comment, we actually could implement There's work associated with that, and it wouldn't have to be all or nothing, but want to make it clear that there's a very viable path to making these all top-level commands and not be in a constant fight with core API conflicts. |
@sipsma I find that possible path pretty cool, to be honest. I do think @jedevc 's concern is worthy of discussion - do we want to commit ourselves to parity between the API namespace and CLI namespace, now and forever? I find "yes" to be a plausible answer, but worth discussing nevertheless. Also: if we choose the path of parity ( |
Additional note: I guess aiming for full parity between API and CLI, makes even more sense in the context of #7466 . For example |
Another problem with full parity: how much of the graphql API can I query without running any containers? For example, if I run the CLI on a mac without a working container runtime: how much of the CLI can I actually use? |
Not an argument for or against, just sharing some (potential?) context. I'm imagining in the near future the CLI exposing more features. For example:
With that being said, is a 1:1 mapping going to help (easier to extend the CLI) or get in the way (trying to fit things into modules that shouldn't, making us slower), or neither? |
One thing to be clear about up front: I don't think we should make some 100% set-in-stone rule that "Thou shalt only implement top-level CLI commands as API calls". It's a nice and appealing approach for a number of reasons but we would probably want to approach migrating existing commands iteratively and also leave the door open for pure CLI implementations if/when needed
@aluzzardi Yes I've been thinking about the same recently alongside other refactoring work and these specifically are examples of commands that we almost certainly do want to be in the top-level graphql API, just due to the fact that they require close integration with all the solver+cache stuff that happens in the engine container. So I think it would align with this idea.
Makes sense too, but at minimum the storage parts is also something where we almost certainly need close integration with the solver+cache in the engine container, so no matter what quite a bit of the logic needs to live there. As for the "serve on the CLI-side" part, this is where all our existing functionality around sessions will help us out enormously. We already have the ability to tunnel traffic on arbitrary ports back and forth between the CLI and the engine container (was added for service proxying/tunneling, but @vito's implementation is very generic and can be re-used out-of-the-box for things like this).
One thing, just to be extra clear, we don't have to implement all this as modules (though that's an option). They can be core API calls as desired, which reduces potential sources of overhead when that matters. Also worth mentioning, in addition to the tunneling session functionality I mentioned above, we also already have the ability to read/write/stat arbitrary files on the CLI's host from the engine container, read auth, etc. It's straightforward to add more functionality as needed too. So I don't think it would get in our way in that sense. The impact on performance is the main thing to look out for though I agree. There's a couple things that lessen my concern somewhat:
If you're asking about what parts of the current core API are "theoretically" usable if they were running directly on macos/windows w/out container support, then the main things hard excluded at the moment would be:
There's a lot of functionality around just pulling images, performing file operations on them, etc. that don't need any containers per-se, though it's going to be hit or miss which parts of some of those currently compile on non-Linux.
Not sure if that answers your question, let me know. |
Problem
There is no way to call core functions from the CLI
Solution
Allow calling core functions from the CLI.
See https://daggerverse.dev/mod/github.com/shykes/core for a userland implementation.
The text was updated successfully, but these errors were encountered: