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 · 27 comments · May be fixed by #7310
Open

Allow calling core functions from the CLI #6947

shykes opened this issue Mar 26, 2024 · 27 comments · May be fixed by #7310
Assignees
Labels
area/cli All about go code on dagger CLI DX+

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli All about go code on dagger CLI DX+
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants