0.8.0 - BREAKING CHANGES RELEASE - Scope, Timeline + Q&A - ETA August 3, 2023

[gerhard]

Scope

0.8.0 BREAKING CHANGES RELEASE - 🐙 GitHub Milestone

Why are we doing this?

Since we shipped SDKs in November 2022 - 🐹 sdk/go/v0.4.0, 🐍 sdk/python/v0.1.1 & ⬡ sdk/nodejs/v0.1.0 - we announced numerous deprecations and discussed few breaking changes in 🐙 GitHub issues & 👾 Discord, but strived to not break anything (unless absolutely necessary).

As a rule of thumb, we try to make breaking changes infrequently & batch them together so that all the disruption happens all at once, rather than a little every few weeks.

Six (6) months later, the time has come to remove all those deprecations and introduce a few breaking changes. This is necessary to make a number of upcoming feature easier & implement the hard-earned feedback from our users. These breaking changes will affect the 🚙 Engine + 🚗 CLI & also all SDKs.

The goal of this discussion is to:

• propose a timeline for this breaking changes release
• discuss what is in scope
• and most importantly, manage the surprise factor

There are many moving parts, different opinions & we all have summer holidays to look forward to, so we want to make the best of this period in the year 🏝 ⛰ ⛵️

What is the expected timeline?

We would like to make these breaking changes releases public at the beginning of August, which is in ~4 weeks time. This is what a rough timeline looks like (red line is today):

gantt
    title 💥 BREAKING CHANGES RELEASE
    dateFormat  YYYY-MM-DD
    axisFormat %b %e, %Y

    section DISCUSS
    ✍️ Add your comments below :2023-06-28, 30d

    section IMPLEMENT
    💻 Make the changes :2023-07-24, 10d
    

    section RELEASE
    🚀 RELEASE :milestone, 2023-08-03
    📝 Blog post :2023-08-03, 3d
    🔧 Follow-up fixes :2023-08-03, 8d

This gives us enough time to spread the message, have the necessary conversations & start implementing the changes. It also works well with various summer holidays that people have coming up. We want to be around for the next 2 weeks after the breaking changes release to quickly address the aftermath.

Other things that went into considering this timeline:

• documentation changes (especially guides)
• more time will tempt us to increase scope which will make it more difficult to release
• we don't want to introduce every breaking change that we can think of (it will make for a hard upgrade)
• we have new features coming up that will need to be layered on top of these breaking changes

Questions & Answers

Do we skip a regular release (usually every 2 weeks), or merge in a different branch than main?

We will merge changes into main as we do for all other releases, no need to delay or use a different branch.

Will the Engine + CLI & SDKs all have the same version?

Yes. Before this release, all our artefacts had the v0.6.x version, except the Go SDK which was at v0.7.x.

Why are we bumping two minor versions, i.e. 0.6.x -> 0.8.0?

To signal that this is a breaking changes release. It also gives us the opportunity to sync the minor version across all artefacts: Engine + CLI & all SDKs.

What are all the places that we need to announce this?

• https://discord.gg/dagger-io
• https://dagger.io/blog

Who owns the announcement blog post?

@helderco

---

What are we missing? Any concerns and/or constructive feedback? Please share it all below 👇

cc @dagger/maintainers

[helderco]

Thanks for this @gerhard 🙌 I agree with the timeline.

• Do we skip a regular release (usually every 2 weeks), or merge in a different branch than main?

Using a different branch seems safer to me. That allows us to push an hotfix release if we need to.

Non-breaking?

We can do the following in a non-breaking change, but perhaps the best UX would be a breaking one so I need to revisit that soon and I’ll update my opinion on it:

• Handling Binary Data #5069

Needs feedback

For clarity, the following two are mutually exclusive. One is for fixing exitCode the other is for deprecating it, so first we need to agree on which to do.

I proposed to deprecate it (rationale is in the issue) however would we skip deprecation for this release and just remove it? Removing is a bigger change for users. Deprecating now is a non-breaking change and we could remove it in the next breaking release.

• Container { exitCode } doesn't work if the command fails #3192
• Deprecate exitCode #5124

This one has a much better UX on returning lists of objects, but it requires feedback because of an implicit/explicit aspect and how it can affect future changes to the API:

• Proposal: Codegen: Return simple objects #4920

More to add

I’d like to add this trivial change to Python:

• Python: force keyword arguments for optional parameters #4182

I remember the discussion on being better to be more explicit with the name envVariable instead of just env, but the long names have pushed some users to ask for some sort of convenience to make their pipelines less verbose. This was discussed in the following issue where @vito suggested that renaming these fields was the simplest solution and I agree. This can go through a deprecation path first though:

• ✨withEnvVariables #4835 (comment)

There’s no issue for this yet, but recently with @sipsma, we made changes to codegen to be able to get an instance of a type if you only have the type name and its ID scalar (e.g., Directory → Query.directory(id: DirectoryID)). There’s an exception with CacheVolume though. It uses a key: String! argument instead of id: CacheID! (i.e., Query.cacheVolume(key: String!):

• Fix hacks in project commands and sync #5353 (comment)

We should find a way to standardize this and use this opportunity to possibly make a breaking change here.

Deprecations to remove

All current API deprecations are pretty old:

• Container.exec (replaced by Container.withExec since v0.3.6)
• Container.fs (replaced by Container.rootfs since v0.3.6)
• Container.withFS (replaced by Container.withRootfs since v0.3.6)
• Directory.workdir (replaced by Host.directory(path: ".") since v0.3.6)
• File.secret (replaced by Client.setSecret since v0.5.0)
• HostVariable.secret (replaced by Client.setSecret since v0.5.0)

Using the Python SDK without async has been deprecated since v0.6.0 (May 25, latest is v0.6.2). Can we remove it on this release or is it better to wait another 6 months? The motivation for this removal and migration guide are explained in the issue:

• Deprecate sync Python #5192

[helderco]

Apart from #5069 which is a new feature, everything else I mentioned above is trivial to implement. And for bigger user changes like renaming envVariable and deprecating exitCode, we can still go through the deprecation path but still take the opportunity of this announcement for users to also want to make those changes but with less pressure from a removal.

[helderco]

Better to hold off on renaming envVariable (etc), because of the new Environment focus on Zenith.

[helderco]

Another issue to add:

• Node.js: Export ´Client´ as a named export, instead of default #5434

[gerhard]

Having talked to @mircubed about the versioning bump, we are thinking bumping 2 minors for all artefacts (0.6.x -> 0.8.0) apart from the Go SDK which we will bump from 0.7.x -> 0.8.0. This means that all versions will line up after this release. cc @d3rp3tt3 @shykes

[shykes]

I’m fine with that. My concerns are 1) marketing the release properly with feature highlights, good blog post and 2) what is the highlight beyond breaking changes?

[gerhard]

New features will be only accidental - i.e. what was merged in between releases - nothing specific.

@helderco owns this blog post, we talked about opening an early draft PR.

We are in the last week before this goes public, ETA August 3th, 2023. I have just updated the title to emphasise this part.

[shykes]

Generally speaking I don’t see any benefit to batching breaking changes together in one big “breaking” release.

• I’m ok moving forward with this plan
• But in the future it seems simpler to just manage each breaking change individually by simply tagging along with the next minor (for 0.x) and later major (for 1.x).

[gerhard]

WDYT @aluzzardi?

[sipsma]

One additional breaking change I think we should consider as part of this is to remove Host.envVariable

Reasons to remove:

1. In practice, due to design changes, it has ended up not being needed anymore and thus confusing to users
   • The original idea from way back to the original cloak release was that extensions would use this to read env vars from the host, but that is no longer the case with environments (where any variables like that are instead explicitly passed around as strings). It was also briefly used to turn an env var into a secret but that's now deprecated and to be removed in this release.
   • I've seen users asking why they should use that instead of their languages normal way of reading env vars (e.g. os.Getenv in go). The current answer is that there is no reason, which is obviously quite confusing.
2. It throws a medium size wrench into the re-architecture of moving the graphql server to the engine runner
   • It's possible to support, but will require either blindly sending every client env var to the server (which just feels like a security bug waiting to happen) or setting up our own custom session attachable for reading arbitrary env vars from clients (which is both complex AND feels like a security bug waiting to happen)

Reasons NOT to remove:

1. We have not actually deprecated it yet; we've only officially marked the HostVariable.secret field as deprecated.

---

Overall, I think the reasons to remove outweigh the reasons not to and we should thus remove it in this release, but the fact that we haven't deprecated officially yet is not to be discounted; in an ideal world we should strive to always follow the process of marking as deprecated for a while and then removing. This feels like it may be an exceptional case, but certainly highly debatable.

Another possibility would be to deprecate it in this release and then, to avoid the problems w/ the re-architecture, we update our SDKs' codegen to just call the language's stdlib to get the env var rather than implement it as a real graphql query? Just thinking out loud as I type this 🙂

WDYT @helderco @gerhard @vito et al.

[helderco]

Totally agree, we should just remove it.

[vito]

+1

Another option: could we ship v0.8.0 without the re-architecture, deprecate Host.envVariable in that release, and then remove it in v0.9 with the new architecture? The new services API won't make it in to v0.8.0 (it's possible but doesn't seem worth the extra effort vs building on top of the new architecture), so we're going to have two breaking releases either way.¹

The question I suppose is whether we hold off on merging the re-architecture or if we make a v0.8.0 release branch that we backport things to until release.

Footnotes

1. Technically there's no deprecation window for the services API change as it's been an experimental feature this entire time, and it's not easy to support the old and new API alongside each other because Container.withServiceBinding takes a different type (Container -> Service). ↩

[sipsma]

Another option: could we ship v0.8.0 without the re-architecture, deprecate Host.envVariable in that release, and then remove it in v0.9 with the new architecture? The new services API won't make it in to v0.8.0 (it's possible but doesn't seem worth the extra effort vs building on top of the new architecture), so we're going to have two breaking releases either way.1

(Been talking to Alex about this on Discord, but repeating for the visibility of others) We're still striving for the re-architecture PR to be part of v0.8.0 by backporting a few pieces of the services v2 implementation to it (no user-facing API changes, internal stuff only), but either way if there's no objections (seems like there's none), I think we should just go ahead and remove the host variables as part of this PR: #5512

@helderco happy to push that change (just the removal of host vars, not all the other stuff) on top of your PR if you'd like

[gerhard]

💯 from me for removing Host.envVariable

[helderco]

Now that this has been brought to my attention I realize it's quite a breaking change since it touches some common fields!

• ✨ EnvVariable should return nil if variable does not exist #5449

The cruft of it is that in the Go SDK, when you use container.EnvVariable or container.Label, the return type in the API is String but in go it's string. So it doesn't allow us to make a distinction between a non-existent value and an empty one. Changing optional leafs to pointers would fix that.

However, it touches several other fields that would be turned into pointers as well:

• Container.EnvVariable
• Container.ImageRef
• Container.Label
• Container.User
• Container.Workdir
• Port.Description
• (other Zenith fields, e.g., command's description...)

This isn't something we can deprecate, we need to do it now, don't you agree?

\cc @sipsma @vito @aluzzardi

[vito]

Personally, I'm OK with these returning zero-values in Go; it feels more conventional than using pointers everywhere. I don't think the distinction between "not set" and "empty value" is important enough in practice, but I'm open to being wrong on that if there's a solid example. I think you could make an argument for env vars being unset vs. empty but even outside of Go I find it's safest to just never make a distinction there, and just set a dummy value like 1 to be super explicit.

An additional rule of thumb would be that if the zero-value isn't valid, we don't need it to be a pointer. Adding pointers for things like Workdir or User might just lead to folks taking shortcuts and dereferencing them, possibly resulting in a cryptic panic. Also typically when you use pointers in this way the API would also provide a helper to return the zero-value form, sort of like how Protobuf generated code has Foo *string and GetFoo() string. It makes a lot more sense in Protobuf where you explicitly have to deal with optional values, but even then I often find myself just using the GetFoo() form.

So: my vote is to leave this as-is.

[helderco]

I flagged this issue initially because in v0.2 (CUE) having this distinction was important to users for some use cases and it's not like a user can always control what the value is set to. It led to several requests and support issues over time. However, that was mostly in the Client API for reading host env variables to setup the Dagger Plan dynamically and it was further complicated because of the string | Secret disjunction.

Now you have several workarounds:

• For host env vars, you just use os.LookupEnv
• In a container you can use the shell to check for an env var (printenv, test, etc...)
• You can also look in EnvVariables to confirm if it's in the container metadata (ImageConfig)

After this was brought to my attention now I went looking for what else would be affected by this and I agree that I much rather have the empty value in the other fields, except for Label. Label is in the same boat as EnvVariable because it's also from a map, but it equally has a Labels list if you really need the distinction.

So it's like we're giving Getenv but you can make your own LookupEnv based on the list.

Thanks for the feedback @vito 🙏, I'm quite convinced we should leave it as-is too. 👍

[gerhard]

This is now out. Thank you all for a massive effort 🙌

All follow-ups are now in:

• v0.8 is now available - how can we help? #5584