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

Usability issues with the Target type #820

Closed
edmorley opened this issue Apr 30, 2024 · 0 comments · Fixed by #821
Closed

Usability issues with the Target type #820

edmorley opened this issue Apr 30, 2024 · 0 comments · Fixed by #821
Assignees
Labels
breaking change enhancement New feature or request

Comments

@edmorley
Copy link
Member

edmorley commented Apr 30, 2024

The distro_name and distro_version fields of the libcnb::Target type are currently optional (Option<String>).

It was implemented this way because the Buildpack API spec says that the CNB_TARGET_DISTRO_NAME and CNB_TARGET_DISTRO_VERSION env vars are optional:
https://github.com/buildpacks/spec/blob/buildpack/0.11/buildpack.md#targets

However, in practice these env vars will always be set, since:

  1. If the base image has the io.buildpacks.base.distro.* Docker labels set, then these values will be used (now that CNB_TARGET_DISTRO_NAME and CNB_TARGET_DISTRO_VERSION not set correctly in buildpack env buildpacks/lifecycle#1324 is fixed).
  2. If the labels aren't set, then lifecycle falls back to the values found in /etc/os-release (once the fix for Target distro name/version not derived from /etc/os-release when Docker labels aren't set buildpacks/lifecycle#1336 is released shortly), and /etc/os-release exists for pretty much all of the common Linux distros (xref https://unix.stackexchange.com/q/351557).
  3. Even if neither the labels nor /etc/os-release is found, lifecycle still sets both of these env vars to the empty string anyway, so the env vars will never be unset.

Plus, for buildpacks that include the distro name/version in their buildpack.toml [[targets.distros]], the buildpack will only ever run when CNB_TARGET_DISTRO_NAME and CNB_TARGET_DISTRO_VERSION matches those values (once the fix for buildpacks/lifecycle#1337 is released).

If they were never not set (which can't currently happen anyway), IMO libcnb.rs should be opinionated and reject this edge case for the sake of normalising metadata to make buildpack authors lives easier.

As-is, since both distro_name and distro_version are an Option, it means they are much more annoying to use. For languages that have distro-version specific binaries, the distro name/version has to be used in many places (from layer metadata, cache invalidation explanation messages, error messages explaining why a particular runtime version isn't available "for this OS ({distro_name} {distro_version}" etc) - and in every place I'm currently using .expect() since the scenario can't happen in practice.

As such, I think we should make them String instead of Option<String>.

(Longer term I'd love to advocate for changing the spec upstream to remove the "optional", but we can still improve the UX within libcnb.rs in the meantime.)

GUS-W-15639138.

@edmorley edmorley added enhancement New feature or request breaking change labels Apr 30, 2024
@edmorley edmorley self-assigned this Apr 30, 2024
edmorley added a commit that referenced this issue Apr 30, 2024
* Makes the distro name/version fields non-optional (`String` instead of
  `Option<String>`), since (a) they will never be `None` in practice
  (see #820), and (b) it otherwise causes a lot of unnecessary friction
  for languages that have distro-version-specific binaries and need to
  use the metadata in multiple places in the buildpack.
* Implements `Clone` and `Debug` on `Target`, which unblocks a number
  of use-cases, such as including the target as part of a buildpack
  Error enum variant.

Fixes #820.
GUS-W-15639138.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant