nebari-dev / nebari-dev/data-science-pack

Three structural gaps surfaced by the first external contribution

Open
#86 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
5
Forks
7
Avg merge
1d 20h
Merged PRs (30d)
13

Description

While reviewing #84 (the first cross-repo fork contribution to land in this pack), I went down the rabbit hole of what our PR CI actually exercises, and three things stood out to me that I think are worth a deliberate decision before we onboard more outside contributors.

1. Fork PRs can't get through the image-build workflow

https://github.com/nebari-dev/nebari-data-science-pack/blob/a1e4601df80696c4de46c37cc6e28c46fdb4b822/.github/workflows/build-image.yaml#L56-L66

.github/workflows/build-image.yaml logs into ghcr.io (with GITHUB_TOKEN) and quay.io (with secrets.QUAY_TOKEN), then push-by-digest=true,push=true is hardcoded into the buildx outputs: spec. Neither side works for fork PRs:

  • GITHUB_TOKEN from fork-event workflows is read-only by default — packages: write is not granted.
  • secrets.QUAY_TOKEN isn't exposed to fork-event workflows at all.

So on #84, every image-build leg failed at the login step with ##[error]Username and password required before the build itself ever ran. The cache-write step (cache-to) already conditions on github.event_name != 'pull_request', so the workflow author started thinking about this, but the login + push themselves aren't gated.

Beyond the immediate "it's broken" problem, even if we made fork PRs push successfully, that would push fork-built images to our public quay registry — and we have no good answer for what a contributor might ship in an image. Pushing fork artifacts to a public org-controlled registry is a supply-chain footgun, not a missing feature.

What can we do?
  • Gate the two docker/login-action steps, the push, and the merge job on github.event.pull_request.head.repo.fork != true. Same-repo PRs keep current behavior; fork PRs build (linux/amd64 only, push=false) to prove the Dockerfile/pixi config resolves, then exit.
  • Add a maintainer-triggered workflow_dispatch that takes a fork's commit SHA, builds, and pushes — the maintainer reviewing the diff and clicking "Run workflow" is the human-in-the-loop approval. This covers the "I want to try this fork's image in a test cluster" use case without exposing secrets to arbitrary fork code.
2. `e2e` validates the pinned image, not the PR's change

https://github.com/nebari-dev/nebari-data-science-pack/blob/a1e4601df80696c4de46c37cc6e28c46fdb4b822/.github/workflows/test.yaml#L57-L75

The prep job in .github/workflows/test.yaml reads .jupyterhub.singleuser.image from values.yaml, docker-pulls that exact ref (currently quay.io/nebari/nebari-data-science-pack-jupyterlab:sha-443d1fe), caches it, and side-loads it into each matrix leg's kind cluster. The hub deployment also resolves its image from values.yaml and pulls from quay at deploy time.

That pinned SHA only changes via a separate hand-authored commit — the chore: bump image tags to sha-XXXXXXX pattern (recent examples: 669028c, 6311157, 6f22748, b733e6e). So the lifecycle is:

  1. A PR touching images/** merges to main → build-images.yaml runs on main and pushes the new image as sha-<new>.
  2. Separately, someone opens a bump PR moving values.yaml's tags from sha-<old> → sha-<new>.
  3. Only after that bump merges does the chart actually deploy the new image.

The implication: when the e2e suite runs on a PR that touches images/**, the image it's exercising is the previously-merged-and-already-vetted one, not the one the PR proposes. The current pipeline gives us "does the chart still deploy the last-blessed image," which is a useful smoke test, but it's not catching regressions introduced by the PR itself.

Possible directions:
  • Build → side-load PR's image into kind → override values.yaml at test time. Closes the loop. Cost: PR e2e now depends on the build job's artifact (a few-GB image tar via upload-artifact/download-artifact, or a registry tag), and the matrix prep becomes more involved. For fork PRs that don't push, this still works via the tar-artifact route.
  • Move e2e to the chart-bump PR. Keep PR CI as "image builds + chart lints"; let the bump PR be where we exercise the new image against the chart. Less rework, but loses the per-image-PR signal.
  • Hybrid: keep the current cheap chart-deploy smoke on every PR, plus a deeper opt-in e2e that builds and deploys triggered by a label or workflow_dispatch when a maintainer wants real validation.
3. Every PR rebuilds all five image targets regardless of which paths have changed

https://github.com/nebari-dev/nebari-data-science-pack/blob/a1e4601df80696c4de46c37cc6e28c46fdb4b822/.github/workflows/build-images.yaml#L9-L24

build-images.yaml fires whenever images/** changes and rebuilds jupyterlab-base, jupyterlab, jupyterlab-gpu-base, jupyterlab-gpu, and jupyterhub — multi-arch where applicable. A PR that only touches images/jupyterlab/pixi.toml rebuilds the jupyterhub image and both GPU variants for no reason. GPU images account for most of the runtime.

Options:
  • Path-scoped triggering via dorny/paths-filter (or hand-rolled): emit per-image change flags, pass them as if: guards on each job. Tricky because of the multi-stage Dockerfile inheritance (jupyterlab builds on jupyterlab-base), so we'd need to encode those edges.
  • Single-arch on PRs, multi-arch on main. Cheaper across the board; main still produces real multi-arch artifacts. Independent of #2 and could ship first.
  • Combine with #2: Once we side-load PR images into kind, only build the image(s) the e2e suite actually needs for the changed paths.
Why this matters now

The team has been org-internal until recently — the failure modes here didn't bite because everyone could push, and the pinned-SHA gap was masked by the bump-PR being part of the same person's workflow. With external contributors now arriving (PR #84 is the first), the gap between "PR CI passed" and "this change is actually safe" will become more visible. Worth landing on a design before the next external PR.

Refs: #84.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read .github/workflows/build-image.yaml, build-images.yaml, test.yaml, and values.yaml, then review the failure described in #84. Map the three workflow gaps and compare their stated options with the repository's current image and e2e flow. Done means a maintainer-approved design with clear scope and acceptance criteria before implementation.

Written by the indexing model from the issue text.

Assessment

Tech stack
docker, github-actions, kubernetes
Domain
ci-cd, devops, infrastructure, testing-qa
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.