Rename id argument in Container.withRootfs
#5513
Conversation
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
| @@ -86,7 +86,7 @@ type Container { | |||
| fs: Directory! @deprecated(reason: "Replaced by `rootfs`.") | |||
|
|
|||
| "Initializes this container from this DirectoryID." | |||
| withRootfs(id: DirectoryID!): Container! | |||
| withRootfs(directory: DirectoryID!): Container! | |||
There was a problem hiding this comment.
I don't disagree with this name, but want to flag that other APIs like withMountedDirectory call this source. I think directory is OK here case because there's nothing to disambiguate it against, whereas withMountedDirectory also has a "target" path. But then withMountedDirectory just calls it path anyway so there's no symmetry like source/target. 🤦♂️
Just raising to see what you think. Would it make sense to rename the others to directory too? 🤔 (Not necessarily as part of this PR; approving as-is.)
There was a problem hiding this comment.
Yeah, I initially had source in the issue but then changed to directory to mirror withDirectory(path: String!, directory: DirectoryID!).
So in mounts source is used, in builds context is used and directory outside of that.
Outliers are Directory.diff which uses other but that makes sense, and Project.load, which uses source.
There was a problem hiding this comment.
I'm good with renaming these for consistency though in a follow-up.
* Rename `id` argument in `Container.withRootfs` Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com> * Add change log Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com> --------- Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Closes #4192
This has minimal impact since the renamed argument is required/positional in the SDKs. Will only break if someone's using the argument name directly such as in a GraphQL query string.