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

core: add support for wipe arg to Directory.export #6909

Merged
merged 2 commits into from Mar 22, 2024

Conversation

sipsma
Copy link
Contributor

@sipsma sipsma commented Mar 20, 2024

Related discord thread: https://discord.com/channels/707636530424053791/1220078037542895617/1220078037542895617

By default, exporting a directory resulted in its contents being merged into the target directory on the host, with any files that existed on the host but not in the source dir being untouched during the export.

More often than not, this is the behavior you want, but there are use cases where you'd instead like to replace the contents of the host directory entirely such that it exactly matches the source dir being exported. This enables you to e.g. load a dir from the host, delete some files from it and then export that back to the host with those deletes being reflected.

This change adds an arg to Directory.export to enable that behavior.

I really wanted to name the arg merge and default it to true, but it turns out that doesn't really work for optional args since at least the Go SDK can't distinguish between an explicit false value on an optional arg and an implicitly set false zero-value, so it wasn't possible to ever get anything but the merge: true behavior. Didn't check other SDKs but I would imagine at least a few of them have the same sort of problem.

So I fell back to just naming the arg replaceAll and making it trigger the non-default behavior only when set to true. I don't especially love the name; open to suggestions.
EDIT: changed name to wipe as suggested in comments

@sipsma sipsma added this to the v0.10.3 milestone Mar 20, 2024
@sipsma sipsma requested review from vito, helderco and jedevc March 20, 2024 21:06
@sipsma sipsma requested review from a team as code owners March 20, 2024 21:06
@helderco
Copy link
Contributor

How do you feel about just "replace"?

@sipsma
Copy link
Contributor Author

sipsma commented Mar 21, 2024

How do you feel about just "replace"?

My objection to that is the existing default behavior will replace files if they exist both on the host and in the dir being exported, so replace by itself doesn't seem descriptive enough. Obviously replaceAll doesn't fully capture the behavior either though, it's just maybe one iota better.

The overly verbose name that would capture it all is something like makeTheHostDirectoryExactlyMatchTheExportedDirectory. Other alternatives I can think of:

  • includeDeletes
  • clobber - i.e. feel free to remove existing files
  • noMerge
  • exactMatch

Of those, I guess I do like includeDeletes, but can't tell if it's actually better than replaceAll. Let me know what you think @helderco (cc @vito, you're good at naming things 😁)

@vito
Copy link
Contributor

vito commented Mar 21, 2024

Whatever it is, I'm glad it's not the default, because I sometimes export to my home directory, and clobbering that would make me sad. 😄

If you can't tell, I kind of like clobber. It's succinct and scary enough to call attention to it. I could see someone adding replace/replaceAll just because they want it to replace individual files (sort of like how unzip prompts you to replace anything), but really it's nuking the destination first1 and then placing your files there. Clobbering is how I'd usually describe an intentionally destructive operation like that.

Might be confusing to non-English-speaking users, but maybe it's common enough jargon to be recognizeable?

Less-cute options if not:

  • replaceDestination
  • wipeDestination
  • clearDestination
  • emptyDestination
  • "" ...Target
  • "" ...Dest
  • "" ...Path
  • pristine
  • clean
  • reset
  • clear
  • wipe

I kinda like wipe too, but it's still kind of jargon-y. I wonder if this is a case where jargon is just more communicative, and owing to the danger/risk (wiping someone's home dir), we should favor jargon over accidentally making it sound too peaceful/dry.

Footnotes

  1. Or maybe it does some fancy diff thing, not sure

@sipsma
Copy link
Contributor Author

sipsma commented Mar 21, 2024

If you can't tell, I kind of like clobber. It's succinct and scary enough to call attention to it. I could see someone adding replace/replaceAll just because they want it to replace individual files (sort of like how unzip prompts you to replace anything), but really it's nuking the destination first1 and then placing your files there. Clobbering is how I'd usually describe an intentionally destructive operation like that.

Yeah fair point about making the name somewhat scary sounding. That leaves me a bit torn between clobber and wipe (destroy and annihilate would be fun but probably too scary 😄). I think I'll update to wipe just because it's shorter to type than clobber. The point about being overly jargony makes sense too, but hopefully if someone is unfamiliar with the terminology they read the docs for it (🤞, 🤜🪵, etc.)

@sipsma sipsma changed the title core: add support for replaceAll arg to Directory.export core: add support for wipe arg to Directory.export Mar 21, 2024
By default, exporting a directory resulted in its contents being merged
into the target directory on the host, with any files that existed on
the host but not in the source dir being untouched during the export.

More often than not, this is the behavior you want, but there are use
cases where you'd instead like to replace the contents of the host
directory entirely such that it exactly matches the source dir being
exported. This enables you to e.g. load a dir from the host, delete some
files from it and then export that back to the host with those deletes
being reflected.

This change adds an arg to `Directory.export` to enable that behavior.

I really wanted to name the arg `merge` and default it to `true`, but it
turns out that doesn't really work for optional args since at least the
Go SDK can't distinguish between an explicit `false` value on an
optional arg and an explicitly set `false`, so it wasn't possible to
ever get anything but the `merge: true` behavior. Didn't check other
SDKs but I would imagine at least a few of them have the same sort of
problem.

So I fell back to just naming the arg `wipe` and making it trigger
the non-default behavior only when set to true.

Signed-off-by: Erik Sipsma <erik@dagger.io>
Copy link
Contributor

@vito vito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point re: the Go SDK making it impossible to represent a default true value is kinda sad, but not sure what to do about it. Part of me takes Go's side in that it's a whole lot simpler to just always have defaults be false. But it's weird that we don't enforce this across languages. And I'm not 100% sure we should. A problem for future us, perhaps. Or maybe I'm overthinking it and there's a small tweak we can make somewhere.

That aside, 👍

@@ -80,7 +80,8 @@ func (s *directorySchema) Install() {
dagql.Func("export", s.export).
Impure("Writes to the local host.").
Doc(`Writes the contents of the directory to a path on the host.`).
ArgDoc("path", `Location of the copied directory (e.g., "logs/").`),
ArgDoc("path", `Location of the copied directory (e.g., "logs/").`).
ArgDoc("wipe", `If true, then the host directory will be wiped clean such that it exactly matches the directory being exported, including deleting any files on the host that aren't in the exported dir. If false (the default), the contents of the directory will be merged with any existing contents of the host directory, leaving any existing files on the host that aren't in the exported directory alone.`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ArgDoc("wipe", `If true, then the host directory will be wiped clean such that it exactly matches the directory being exported, including deleting any files on the host that aren't in the exported dir. If false (the default), the contents of the directory will be merged with any existing contents of the host directory, leaving any existing files on the host that aren't in the exported directory alone.`),
ArgDoc("wipe", `If true, then the host directory will be wiped clean before exporting so that it exactly matches the directory being exported. If false (the default), the contents of the directory will be merged with any existing contents of the host directory, leaving any existing files on the host that aren't in the exported directory alone.`),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of like being extra explicit about the fact that it will delete files on the host, so leaving in that extra part but will clarify the phrasing.

@@ -226,7 +227,8 @@ func (c *Client) LocalDirExport(
}

ctx = engine.LocalExportOpts{
Path: destPath,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so this defaulted to 'delete everything' before? I've had files mysteriously disappear recently, maybe this expains it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No the behavior before was to not delete any files, that's unchanged here. So something else must have been afoot 🔎

Signed-off-by: Erik Sipsma <erik@dagger.io>
@sipsma
Copy link
Contributor Author

sipsma commented Mar 21, 2024

The point re: the Go SDK making it impossible to represent a default true value is kinda sad, but not sure what to do about it. Part of me takes Go's side in that it's a whole lot simpler to just always have defaults be false. But it's weird that we don't enforce this across languages. And I'm not 100% sure we should. A problem for future us, perhaps. Or maybe I'm overthinking it and there's a small tweak we can make somewhere.

The only fix I can think of would be to make scalar options pointers, so then we can distinguish unset values from explicitly zero values. And then add some tiny util to the go sdk that uses generics to return a pointer to any value (which wasn't possible at the time the go sdk was originally written back in the day 👴).

That's obviously a super breaking change though, so would either need shenanigans w/ version-conditional behavior or to break everyone...

And then multiply that by the number of other SDKs impacted by the same thing.

@sipsma sipsma merged commit b67fa09 into dagger:main Mar 22, 2024
43 checks passed
@jedevc
Copy link
Member

jedevc commented Mar 25, 2024

Late to the party, but I think I also asked for this at some point 😆 Super nice to see it land, LGTM 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants