Skip to content

Commit

Permalink
simplify module config UX via automatic sparse loading of required fi…
Browse files Browse the repository at this point in the history
…les (#6575)

* fix loading of arrays of primitives from dagql select

Signed-off-by: Erik Sipsma <[email protected]>
Co-authored-by: Alex Suraci <[email protected]>
Co-authored-by: Erik Sipsma <[email protected]>

* fix module include/exclude applying to essential files

This started as a fix to a problem where syncing a module using
"include" in dagger.json was resulting in the dagger.json being reset to
an empty module. The root cause was that the include did not have
dagger.json itself in it, so when the module was loaded into the engine
it looked like an uninitialized module that needed to be initialized.

That bug in particular was a regression caused by the recent changes
around module loading and configuration.

This change fixes that bug, but along the way I realized there's quite a
few pre-existing issues that have existed since forever around
include/exclude too. It's too much to squash all at once, but I went with
an approach that will set us up to fix those in further iterations and
should make it possible to implement some loading optimizations around
huge monorepos we've discussed previously.

The other pre-existing issues are:
* Include/exclude settings apply to source files like main.go,
  src/main.py, src/index.ts, etc.
  * This one in particular is fixed in this change. A new Function,
    `requiredPaths` is added to the SDK interface that lets SDKs tell
    the engine which files should always be loaded.
* Include/exclude settings apply to local dependencies (and local custom
  SDKs), which I'd consider surprising enough to be a bug. It would be
  expected that if you configure a local dependency, that acts as an
  implicit include of it.
* Include/exclude settings of local dependencies are ignored, only the
  top level module's include/excludes are actually impactful in terms
  of what's loaded.
* Exclude in particular overrides any include settings, including all
  essential files (still the case after this change).

The approach I went with here centralizes even more loading logic in the
engine. There's a new field on LocalModuleSource called
resolveFromCaller that uses the filesync API to do all of the find-up
logic, config reading/parsing and directory loading for the caller from
the engine. The CLI (or any other SDK user) can now load modules from
their local filesystem with just that API rather than doing the whole
dance themselves.

This approach is beneficial for a few reasons:
1. There's less logic to keep in sync between the engine and the CLI.
   Similarly, local module loading is re-usable beyond just the CLI now
2. Fixing the problem around files like main.go, src/main.py, etc. not
   being loaded required adding a new function to the SDK interface. The
   CLI could have been exposed to that, but it would have continued to
   make its logic even more complicated, especially around the case of
   custom local SDKs.
3. In the future, fixing the rest of the pre-existing issues listed
   above would also have deepened the complexity of the CLI's logic
   much, much further (while also requiring the engine-side loading of
   dependencies to have its own version of the same logic).
4. This sets us up for optimizations we've discusssed previously around
   loading large sets of dependencies from monorepos, which again
   requires complex logic that's better centralized in the engine.

It's still entirely possible for SDKs to just load up a local directory
as a module if they want; that API is not removed. It's just not
required they implement complex custom loading logic that is required to
get everything working correctly+optimally.

Signed-off-by: Erik Sipsma <[email protected]>

* tests passing

Signed-off-by: Erik Sipsma <[email protected]>

* add support for init --source

Signed-off-by: Erik Sipsma <[email protected]>

* cleanup; pushing to debug dagql error

Signed-off-by: Erik Sipsma <[email protected]>

* dagql: fix selecting objects

This is a bit of hack atm but enough to unblock the only use case for
it. Can be generalized as needed.

Signed-off-by: Erik Sipsma <[email protected]>

* checkpoint passing tests before rebase on main

Signed-off-by: Erik Sipsma <[email protected]>

* backfill docs + sdk codegen

Signed-off-by: Erik Sipsma <[email protected]>

* fix test failures

Signed-off-by: Erik Sipsma <[email protected]>

* fill in comments on subtle parameters/fields

Signed-off-by: Erik Sipsma <[email protected]>

* fix another rebase on main

Signed-off-by: Erik Sipsma <[email protected]>

* switch default source to just "dagger/"

Signed-off-by: Erik Sipsma <[email protected]>

* make --source always rel to cwd

Signed-off-by: Erik Sipsma <[email protected]>

* update help docs from feedback

Signed-off-by: Erik Sipsma <[email protected]>

* cleanup logic based on feedback

Signed-off-by: Erik Sipsma <[email protected]>

* fix case of upgrading from old config

Signed-off-by: Erik Sipsma <[email protected]>

* regen cli docs; rm stray debug println

Signed-off-by: Erik Sipsma <[email protected]>

---------

Signed-off-by: Erik Sipsma <[email protected]>
Co-authored-by: Alex Suraci <[email protected]>
  • Loading branch information
sipsma and vito authored Feb 8, 2024
1 parent 9af955b commit cdaf4e8
Showing 1 changed file with 228 additions and 81 deletions.
Loading

0 comments on commit cdaf4e8

Please sign in to comment.