Skip to content
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

Open
shykes opened this issue Mar 26, 2024 · 39 comments · May be fixed by #7310
Open

Allow calling core functions from the CLI #6947

shykes opened this issue Mar 26, 2024 · 39 comments · May be fixed by #7310
Assignees
Labels

Comments

@shykes
Copy link
Contributor

shykes commented Mar 26, 2024

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.

@shykes
Copy link
Contributor Author

shykes commented Mar 26, 2024

FYI @sipsma, this may be a duplicate, after a quick search I couldn't find a direct equivalent, but may not have found it.

@helderco
Copy link
Contributor

helderco commented May 3, 2024

Would you use -m core to select it? If so, we need to reserve that name:

  • Forbid creating a module with name "core"
  • Forbid installing a dependency with name "core"

@shykes
Copy link
Contributor Author

shykes commented May 3, 2024

Would you use -m core to select it? If so, we need to reserve that name:

  • Forbid creating a module with name "core"
  • Forbid installing a dependency with name "core"

Good question... any thoughts @sipsma @vito @aluzzardi @jedevc ?

@jedevc
Copy link
Member

jedevc commented May 7, 2024

Good point - I like the idea of needing to explicitly select it, so yes, we'd probably need to reserve this ASAP.

@vito
Copy link
Contributor

vito commented May 7, 2024

Reserving core sounds fine to me - it's short and sweet, and I can't think of anything that would conflict with it (in a way that's legitimate and not misleading).

@helderco helderco self-assigned this May 7, 2024
@helderco
Copy link
Contributor

helderco commented May 7, 2024

I have a working POC:

Screenshot 2024-05-07 at 17 08 30

Just needs the name reservation and a bit of polish to reduce what's available:

dagger-dev -m core functions
Name                                   Description
blob                                   Retrieves a content-addressed blob.
builtin-container                      Retrieves a container builtin to the engine.
cache-volume                           Constructs a cache volume for a given cache key.
check-version-compatibility            Checks if the current Dagger Engine is compatible with an SDK's required version.
container                              Creates a scratch container.
current-function-call                  The FunctionCall context that the SDK caller is currently executing in.
current-module                         The module currently being served in the session, if any.
current-type-defs                      The TypeDef representations of the objects currently being served in the session.
default-platform                       The default platform of the engine.
directory                              Creates an empty directory.
file                                   -
function                               Creates a function.
generated-code                         Create a code generation result, given a directory containing the generated code.
git                                    Queries a Git repository.
host                                   Queries the host environment.
http                                   Returns a file containing an http remote url content.
load-cache-volume-from-id              Load a CacheVolume from its ID.
load-container-from-id                 Load a Container from its ID.
load-current-module-from-id            Load a CurrentModule from its ID.
load-directory-from-id                 Load a Directory from its ID.
load-env-variable-from-id              Load a EnvVariable from its ID.
load-field-type-def-from-id            Load a FieldTypeDef from its ID.
load-file-from-id                      Load a File from its ID.
load-function-arg-from-id              Load a FunctionArg from its ID.
load-function-call-arg-value-from-id   Load a FunctionCallArgValue from its ID.
load-function-call-from-id             Load a FunctionCall from its ID.
load-function-from-id                  Load a Function from its ID.
load-generated-code-from-id            Load a GeneratedCode from its ID.
load-git-module-source-from-id         Load a GitModuleSource from its ID.
load-git-ref-from-id                   Load a GitRef from its ID.
load-git-repository-from-id            Load a GitRepository from its ID.
load-host-from-id                      Load a Host from its ID.
load-input-type-def-from-id            Load a InputTypeDef from its ID.
load-interface-type-def-from-id        Load a InterfaceTypeDef from its ID.
load-label-from-id                     Load a Label from its ID.
load-list-type-def-from-id             Load a ListTypeDef from its ID.
load-local-module-source-from-id       Load a LocalModuleSource from its ID.
load-module-dependency-from-id         Load a ModuleDependency from its ID.
load-module-from-id                    Load a Module from its ID.
load-module-source-from-id             Load a ModuleSource from its ID.
load-module-source-view-from-id        Load a ModuleSourceView from its ID.
load-object-type-def-from-id           Load a ObjectTypeDef from its ID.
load-port-from-id                      Load a Port from its ID.
load-scalar-type-def-from-id           Load a ScalarTypeDef from its ID.
load-secret-from-id                    Load a Secret from its ID.
load-service-from-id                   Load a Service from its ID.
load-socket-from-id                    Load a Socket from its ID.
load-terminal-from-id                  Load a Terminal from its ID.
load-type-def-from-id                  Load a TypeDef from its ID.
module                                 Create a new module.
module-dependency                      Create a new module dependency configuration from a module source and name
module-source                          Create a new module source instance from a source ref string.
pipeline                               Creates a named sub-pipeline.
secret                                 Reference a secret by name.
set-secret                             Sets a secret given a user defined name to its plaintext and returns the secret.
socket                                 Loads a socket by its ID.
type-def                               Create a new TypeDef.
version                                Get the current Dagger Engine version.

Including skipping optional flags that aren't supported rather than error.

@shykes
Copy link
Contributor Author

shykes commented May 7, 2024

Would it break github.com/shykes/core? 😭

@helderco helderco linked a pull request May 7, 2024 that will close this issue
2 tasks
@helderco
Copy link
Contributor

helderco commented May 7, 2024

Would it break github.com/shykes/core? 😭

Actually it doesn't. Just need to choose a different name if installing so that you can't use -m core. Even if just checked out locally you could use -m ./core (or an absolute path).

Basically, only the exact -m core needs to be reserved, it's not about the module's name really. Or we can use something else that doesn't clash with anything like -m @. To be clear that syntax is just to prove a point, not an endorsement 😄

@helderco
Copy link
Contributor

helderco commented May 7, 2024

Wdyt of -m dag?

@shykes
Copy link
Contributor Author

shykes commented May 8, 2024

I think -m core is more straightforward than -m dag.

the downside of both options, is that the module name doesn't map to the codegen namespace.

  • install .../docker: dag.Docker()
  • install .../python: dag.Python()
  • install core: dag.Git(), dag.Container()... 🤔

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 -m in the same command , which seems a bit hard to do now, but I kind of wish we had discussed that ise case earlier...

@helderco
Copy link
Contributor

helderco commented May 8, 2024

Let’s see, the main difference when loading core vs a module named “core” is this:

  • -m ./core → preselects query{core{}}, available functions are from Core object
  • -m core → preselects query{}, available functions are from Query object

Just brain dumping here to make a point…

The default for -m is ".", so we could use an empty string to mean core. However that doesn’t look like good ergonomics, and defaulting to the current directory is still a better default than running core. Example:

  • dagger -m "" call container ...
  • dagger -m - call container ...
  • dagger -m _ call container ...

Those would avoid a naming conflict, but are they intuitive? The thing is, I don’t think anyone is going to “guess” to use -m core so you need documentation anyway. The question then is, do they have good ergonomics or do they feel weird? Is there something else that doesn’t feel weird, doesn’t cause a naming conflict and is better at conveying consistency? I think that -m "" fixes the last two but not the first (i.e., weird). If I had to pick one, it would probably be -m - as the - is sometimes used in CLIs to mean something special, although it’s usually for stdin.

End brain dump

As for using multiple -m, I remember us talking about it at some point but at the moment I’m not seeing how that could be used. You have a chain that has to start somewhere (ultimately from Query). If it’s because of multiple queries in one command, how would you separate the chains? With --?

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.

@vito
Copy link
Contributor

vito commented May 8, 2024

Throwing another option into the mix: -m query. That would bring it into alignment with regular module invocation, at least as far as the target type is concerned (Query).

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 -m flags, assuming the first N modules are loaded and the last -m determines the target of the call:

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.

@shykes
Copy link
Contributor Author

shykes commented May 9, 2024

What about -m git, -m container, -m http, -m directory? It would map to the top-level graphql namespace.

@helderco
Copy link
Contributor

helderco commented May 9, 2024

What about -m git, -m container, -m http, -m directory? It would map to the top-level graphql namespace.

I thought about that but we need something to avoid naming conflicts like -m :container or -m .container. I like the latter but it's too close to a local path.

@shykes
Copy link
Contributor Author

shykes commented May 9, 2024

we need something to avoid naming conflicts like -m :container or -m .container.

What's the naming conflict we need to avoid?

@helderco
Copy link
Contributor

helderco commented May 9, 2024

Just disambiguity. If you have a core directory, does the CLI use the core api or that directory? Makes more sense to select the core API because you can do -m ./core (like docker named volumes). Still, can be a bump in the road when it happens. What about a dependency named core?

Just trying to avoid having to reserve a name if there's something we can use to disambiguate.

@helderco
Copy link
Contributor

The output in #6947 (comment) has been reduced because of

So all that remains is to finish bikeshedding on what value to use for -m. I've been using dagger -m- call and I like it. You can try it in:

The - value (aka, blank) is simple and avoids the need to reserve a module name or do any special handling in trying to detect if it's a local path, a dependency name, etc...

You can use -m - but there's a balance to -m-.

@shykes
Copy link
Contributor Author

shykes commented May 21, 2024

I prefer -m container, -m git, -m http.

cc @sipsma

@helderco
Copy link
Contributor

I prefer -m container, -m git, -m http.

I see too problems though:

  • Can't list available core functions
  • Possible naming conflict that needs to be guarded against1

Footnotes

  1. For example, a local path or dependency name targetting a module with a different name. That would be confusing for the CLI, but doesn't produce an error today. Related: Type name conflicts - #6952.

@shykes
Copy link
Contributor Author

shykes commented May 21, 2024

  • Can't list available core functions

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.

  • Possible naming conflict that needs to be guarded against1

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 dag.Container() from my code? If we enable -m container, wouldn't that just surface in the CLI the same problem that we already have?

TLDR: yes name conflicts with core modules are a problem, but I think a problem we already have and should solve separately.

@helderco
Copy link
Contributor

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 dag.Container() from my code?

I put in a footnote that you wouldn't be able to create a module with that name but you can:

  • Have a subdir called container while the module's name in dagger.json is my-container
  • Install a my-container module, but override the dependency's name to container

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?

  • Try loading as usual unless the module can't be found and only then fallback to look for core functions?
  • Check if there's a core function first, before trying to load a user module?
  • Check for both and return an error if both exist?

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.

@shykes
Copy link
Contributor Author

shykes commented May 21, 2024

you wouldn't be able to create a module with that name but you can:

  • Have a subdir called container while the module's name in dagger.json is my-container
  • Install a my-container module, but override the dependency's name to container

What's better?

  • Try loading as usual unless the module can't be found and only then fallback to look for core functions?
  • Check if there's a core function first, before trying to load a user module?
  • Check for both and return an error if both exist?
  • In the subdirectory case: I think core should win. You can always disambiguate with ./container
  • In the "name override" case: what's the current behavior when I try to call dag.Container() in code? Does the custom dependency win, and core gets hidden?

@helderco
Copy link
Contributor

helderco commented May 21, 2024

  • In the "name override" case: what's the current behavior when I try to call dag.Container() in code? Does the custom dependency win, and core gets hidden?

If the module with the dependency has sources, it'll fail to dagger develop so that's only plausible in a dagger.json that's only used to get shorthand names for -m on a few modules.

@shykes
Copy link
Contributor Author

shykes commented May 21, 2024

  • In the "name override" case: what's the current behavior when I try to call dag.Container() in code? Does the custom dependency win, and core gets hidden?

If the module with the dependency has sources, it'll fail to dagger develop so that's only plausible in a dagger.json that's only used to get shorthand names for -m on a few modules.

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 -m container, -m git, -m http are worth it.

WDYT?

@helderco
Copy link
Contributor

I'll just look for a match under Query first, and fallback to the default behavior for loading modules that exists today.

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 -m container is technically more equivalent to -m my-module because they are both selecting a field directly under "Query", but there's other inconsistencies.

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 --mod container is selecting the container module from the core api, rather than selecting the container function from the core module?

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 -m version, the constructor returns a string so dagger -m version call returns the final output.

There's also the set-secret "module" and the load-secret-from-id "module", etc.

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.

@jedevc
Copy link
Member

jedevc commented May 22, 2024

Personally, I kind of think core as a module, and container and directory, etc, as functions in that module.

Here's a few concrete reasons I think of it that way:

  • There's so many links between these functions - I can load files/directorys into containers, I can turn containers into directorys (with rootfs), etc. No other module is currently allowed to talk directly with types like this - we require that they use interfaces instead - this restriction isn't likely to be lifted anytime soon, because of weird versioned dependency issues.
    • Now these modules are ultra-special - instead of one special core module, we have several special modules. If we ever want to try splitting them out properly (which we have discussed as potential future work before I think), having all of these links is going to make this difficult.
  • Some types don't fit neatly into an existing module. For example, platform, which is a top-level type. Does this go in the container module (where it's used the most), or does it live by itself in a tiny top-level module? Does that apply to every enum (if so, that could be a few).
  • Adding top-level fields (like version, as @helderco mentioned above) is weird. We could entirely rewrite the entire core API structure to avoid these, but that feels out of scope for right now, IMO.

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 core/./- module today, and then later (if we want to), splitting these out into separate modules?


In regards to where we attach the methods - I could imagine a future deprecation where we move all of our core module methods to Query.core at a graphql level, so doing dag.Core().Container() instead of dag.Container().

@helderco
Copy link
Contributor

helderco commented May 22, 2024

Is there anything stopping us from having a core/./- module today, and then later (if we want to), splitting these out into separate modules?

Not technically.

Personally, I kind of think core as a module, and container and directory, etc, as functions in that module.

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.

Rationale

It’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 dag.Core(), then I’d say that’s the time to also change -m container to -m core container as it’s going to require a change in user code anyway and this way it matches more cleanly.

Another way to solve the mismatch is to decouple module loading with constructor selection. The -m flag was created to load a module so that it expands the API schema. But when I created dagger call I thought it would be a waste to repeat the main object’s constructor (function under Query), so the CLI conveniently assumes they’re the same:

- 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 --dag flag for the latter (to match the SDKs):

dagger --mod=parrot call --dag=parrot echo --msg=hello

This way, if the default for --dag is the module’s constructor, the current usage stays the same:

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 --mod defaults to ., you may still be loading a module unnecessarily in order to call a core function, so you actually want to tell the CLI not to load any module:

dagger --mod="" call --dag=version

But… at this point, if every time you need --dag, you also probably want --mod "", then why use --dag at all?

On one hand, the CLI could clear --mod automatically if --dag is set. On the other, an empty --mod value could mean the same thing:

dagger --mod="" call version

So --mod basically:

  • If set, load module and select its constructor
  • If not set, don’t load any module and don’t select any constructor

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 -m behind call like I usually do, it’s weird for constructors that return scalars:

dagger -m version call

More of a stretch to explain.

Bikeshedding options

  • dagger call --dag=version
    • Decoupled from module loading
    • Consistent with dag.Version()
    • Sets --mod "" automatically
    • Pitfall: people may try dagger call --dag=trivy scan
      • Assuming current module at . is trivy
      • Won’t work if -m is cleared automatically
      • Possible if dagger -m trivy call --dag=trivy scan which is a waste because it’s the default. Perhaps useful in a shell script though.
    • Unnecessary abstraction, easy to waste resources (due to pitfall)
  • dagger call --dag version (boolean)
    • Same as previous but without pitfall
    • Boolean flag, meaning: select from root, rather than the module’s constructor
    • Keeps version as the first function in the chain, rather than a value of --dag
    • Allows listing all core functions (dagger call --dag)
  • dagger -m "" call version
    • Blank module, meaning: don’t load any module, therefore no pre-selected module constructor
    • Allows listing all core functions
    • Weird to type the quotes, but intuitive in meaning
  • dagger -m- call version (or dagger -m - call version)
    • An alias to -m "" for better ergonomics
  • dagger -m call version
    • Yes, it’s possible to specify -m without a value, and make that mean ""
    • Possibly confusing to users (e.g., the module is call?)
    • Only feasible if -m is put behind call though, unless we introduce -- to split which arguments depend on loading functions
      • May also be a solution to prevent function argument name conflicts (e.g., verbose, output)
  • dagger -m version call
    • Requires that we think of each core function under Query as a constructor to an imaginary module with no clear boundaries.
    • Can surface some inconsistencies simply because the core API isn’t yet built like a module.
    • Violates the assumption (for modules) that a module’s constructor returns the module’s main object. Core module constructors may in fact return a scalar.
    • Less performant because it requires an extra API query for a check and fallback loading logic.
    • Doesn’t support listing core functions under Query.
    • Best ergonomics, but only if -m is used after call
      • Considering it’s generally appreciated before it (me included)
      • Only for certain types like container or directory.
      • Can also be the worst, for example, with functions like version that return a scalar.

After re-considering those options, I’m leaning more towards -m "" again (with the - alias). It has a simpler implementation, it’s more performant, more consistent overall, generally better ergonomics and easier to explain.

Using --dag as a boolean is a close contender for me as well (or something similar, including shorthand -d).

@jedevc
Copy link
Member

jedevc commented May 28, 2024

Out of all the options, I'm happy with any of -m "", -m "core", -m "-". I think that's what @helderco is also leaning towards, so I think I'm in agreement.

I would rather not have --dag, I think that adds a log of confusion beyond what we have today.

I would rather not have -m container and such, I've still got the opinion from #6947 (comment).

@vito
Copy link
Contributor

vito commented May 28, 2024

Also good with -m- - same rationale as @jedevc; because the types are so inter-dependent, the built-in APIs feel more like a 'core' module than individual modules.

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.

@shykes
Copy link
Contributor Author

shykes commented May 28, 2024

  • It seems there is consensus for discarding -m container, -m directory etc.
  • -m core feels wrong to me, because the core API is not in fact a module. It looks a lot like a module, but there are subtle mismatches which make it a leaky abstraction to pretend it is one.

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:

  1. A dedicated flag, for example dagger call --no-modules, dagger call --core or equivalent
  2. Dedicated commands. If container(), git() and directory() are in the core API, why not have dagger container, dagger git and dagger directory?

Arguing for dedicated core commands

dagger call is designed specifically for modules: to call functions which we don't know about in advance. This requires jumping through design hoops.

Core functions are different, they are baked in - maybe we don't need to jump through the hoops in the first place?

Feature API CLI
Native support for containers container() dagger container
Native support for directory artifacts directory() dagger directory
Native support for git git() dagger git
Extend the API with modules MODULE().FUNC() dagger call -m MODULE FUNC

@sipsma
Copy link
Contributor

sipsma commented May 28, 2024

(jumping in here way too late, apologies!)

I originally liked -m container, -m directory, etc. but @jedevc's points do make the circle particularly hard to square. There's probably paths but it devolves into too much special-casing and general weirdness.

-m core feels wrong to me, because the core API is not in fact a module. It looks a lot like a module, but there are subtle mismatches which make it a leaky abstraction to pretend it is one.

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" (dag.Container() vs. dag.Core().Container(), etc.). Maybe we could just special case a core module in this one exact way, but otherwise we'd need to add some general support for any module to have this ability, which feels like too huge a one-way-door to open.

Dedicated commands. If container(), git() and directory() are in the core API, why not have dagger container, dagger git and dagger directory?

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 dagger CLI commands, but that's a small enough thorn that we can deal with it hopefully.

  • We also don't need to necessarily expose all the top-level core APIs this way; we can pick and choose what makes sense if needed.
  • I think this is also helped quite a bit by the fact that we generally speaking should be much more biased towards always implementing something as a module going forward, only adding to the top-level core API when truly necessary.

An intriguing side-effect of this is that it almost makes the entire CLI programmable as just the dagger API.

  • E.g. would it make sense for dagger login/dagger publish/etc. to be implemented as a core API call? I have no clue yet, but it would become a more obvious possibility now and is interesting in some ways.
  • It sort of aligns with some of the work on module loading a few months back to centralize as much of the logic and functionality in the engine container, just giving the container the ability to interact with the client remotely (e.g. stat/read files, etc.)

Thinking out loud some more, it's also almost tempting at that point to support:

  • dagger container
  • dagger directory
  • dagger git
  • dagger -m MODULE FUNC
    • basically, allow skipping call when -m is specified (though we can leave in call for backwards compatibility)

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 😅

@jedevc
Copy link
Member

jedevc commented May 29, 2024

Hm, if we do this, we already have a conflict - dagger version is both already a cli command, and with this would be an API call. Potentially we can special case this in the CLI to do both, which would be pretty neat, but conflicts are now very real and every new cli+core change needs to be considered for this same thing.

To be honest, I quite liked when we switched away from all the previous "verbs" of the old cli, and moved to everything under call - it made for a smaller-feeling, easier to navigate CLI (not quite sure why I like this, it just feels right). I think of dagger as the root command, call as the action, and what comes after as instructions for that action. Same as with functions, config, etc. Moving our core API to the top-level confuses that - container isn't an action, it's a core type - what are we doing with it?

dagger call is designed specifically for modules: to call functions which we don't know about in advance.

I think I disagree - I think call is for calling things. You can call either a user module or the "core" module. Internally, as well as externally, I think these are "kind of" the same thing - it's all just dagql under the hood.

If we split these functionalities out, note that now you can interact with the core API in a couple ways:

  • dagger call -m my-module make-me-a-container with-exec ...
  • dagger container from --address=my-image with-exec ...

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 login/logout, etc.

@shykes
Copy link
Contributor Author

shykes commented May 29, 2024

Hm, if we do this, we already have a conflict - dagger version is both already a cli command, and with this would be an API call.

This particular conflict is fine I think. We are not obligated to map everything 1-1.

Potentially we can special case this in the CLI to do both, which would be pretty neat, but conflicts are now very real and every new cli+core change needs to be considered for this same thing.

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".

To be honest, I quite liked when we switched away from all the previous "verbs" of the old cli, and moved to everything under call - it made for a smaller-feeling, easier to navigate CLI (not quite sure why I like this, it just feels right). I think of dagger as the root command, call as the action, and what comes after as instructions for that action. Same as with functions, config, etc. Moving our core API to the top-level confuses that - container isn't an action, it's a core type - what are we doing with it?

No question, dagger ACTION is nice and simple, and also familiar.

But dagger THING ACTION would also be very familiar. And mixing the two patterns is also very common. So I don't think there would be confusion. See docker run and docker container xxx... :)

Separately from this, I think it's inevitable that we'll need dagger THING ACTION somewhere in our CLI. In particular I think dagger develop will need to be split up into more precise subcommands, possibly under dagger sdk since all subcommands would interact with the SDK.

dagger call is designed specifically for modules: to call functions which we don't know about in advance.

I think I disagree - I think call is for calling things. You can call either a user module or the "core" module. Internally, as well as externally, I think these are "kind of" the same thing - it's all just dagql under the hood.

Right but "kind of" is the whole problem. call was designed in a way that it doesn't fit perfectly with the core API, because of mandatory -m and the expected behavior of auto-calling module entrypoints.

If we split these functionalities out, note that now you can interact with the core API in a couple ways:

  • dagger call -m my-module make-me-a-container with-exec ...
  • dagger container from --address=my-image with-exec ...

I like this because it emphasizes that one is an external module, and the other is builtin.

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 login/logout, etc.

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

@sipsma
Copy link
Contributor

sipsma commented May 29, 2024

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 login/logout, etc.

Here's a possible alternative, for preventing "proliferation" of the call syntax everywhere in the CLI:

As I mentioned in my last comment, we actually could implement login/logout as core APIs. I think almost all of the top level commands could be if we wanted (e.g. even something seemingly complicated like listen would just use the existing network proxying session attachables).

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.

@shykes
Copy link
Contributor Author

shykes commented May 29, 2024

@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 (dagger container, dagger git etc), what kind of parity should we aim for? Should we aim for a parity so complete that all CLI commands could one day be generated from the API schema, like we do for call? Or is it OK to always have some amount of handrolling? And if so, how much handrolling?

@shykes
Copy link
Contributor Author

shykes commented May 29, 2024

Additional note: I guess aiming for full parity between API and CLI, makes even more sense in the context of #7466 . For example version would no longer be split, if there's only one version to be worried about.

@shykes
Copy link
Contributor Author

shykes commented May 29, 2024

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?

@aluzzardi
Copy link
Member

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:

  • Run Garbage Collect (for storage drivers)
  • Other storage-related commands. No idea what but it's possible something will need to be exposed
  • Serve a "storage service". Don't know what it is, but if the storage architecture takes us somewhere centralized (e.g. database backed layer metadata storage), then we may need to run a service, and we may want to bake it into the CLI
  • Control Plane: maybe in the future the CLI will be able to listen to GitHub etc events directly. We may need a command to listen for such events

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?

@sipsma
Copy link
Contributor

sipsma commented May 30, 2024

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

  • But if we have a single binary this all becomes a bit more moot like @shykes said

Run Garbage Collect (for storage drivers)
Other storage-related commands. No idea what but it's possible something will need to be exposed

@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.

Serve a "storage service". Don't know what it is, but if the storage architecture takes us somewhere centralized (e.g. database backed layer metadata storage), then we may need to run a service, and we may want to bake it into the CLI
Control Plane: maybe in the future the CLI will be able to listen to GitHub etc events directly. We may need a command to listen for such events

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).

  • We can also easily add variations on this that don't require localhost to be involved and instead just plumb e.g. github events to the CLI directly over a socket, which probably makes more sense for those specifically

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, 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:

  1. We already have to do all this "session callback to client" stuff for tons of really basic functionality, so if we do need to tune performance somewhere, it's likely going to benefit everything rather than just be a thorn to deal with to support the idea in this issue.
  2. All the refactoring I've been doing has ended up entangling simplifications that reduce the overhead of all this (e.g. less layers of gRPC tunneling.), and it's also making it easier for us to iterate on all of this more as needed.

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?

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:

  • withExec + services
  • modules (since they currently rely on running in containers)

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.

  • There has actually been a ton of work in buildkit on native windows support (and some on native mac os support), so it would mostly be a matter of whack-a-mole on using go build tags in the right places so code actually compiles such that it's using the right underlying syscalls.
  • But for other gql API calls that don't involve syscalls at all, or only minimally (e.g. checkVersionCompatibility comes to mind first) this shouldn't be a problem

Not sure if that answers your question, let me know.

@samalba samalba removed the area/cli All about go code on dagger CLI label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants