Repository navigation
Push frontend images to ghcr (pr-N/staging from PRs, main/sha from main) - #22
Conversation
… from main Images are ghcr.io/<org>/<id>-frontend for both families (moleculemaker for alphasynthesis, ibiofoundry for alphasynbio), derived by app:sync and logged in with GITHUB_TOKEN, so no registry secrets. Tags follow the agreed scheme: a PR build pushes pr-<N> and staging; a main build pushes main (prod) and sha-<short>; nothing pushes latest. The release trigger, whose builds never pushed, and the ignored configuration build-arg are gone. The chart-values tests still compare against ezspecificity and mutagenesis; the image is now the only expected difference for both. Closes #13
matthewberry
left a comment
There was a problem hiding this comment.
Review of 2005ea6.
Verified
- The tag rules match the table. I read
is_default_branchin metadata-action v5.10.0 (src/meta.ts:444). It comparescontext.refwithout therefs/heads/prefix to the repo's default branch. Onpull_requestthe ref isrefs/pull/N/merge, somainandsha-<short>are off, andstagingpluspr-Nare on. On a push tomainit's the reverse.flavor: latest=falsestops the automaticlatest. - This PR's run.
dockerandE2E on Chromeboth pass. The packageghcr.io/moleculemaker/test-frontendexists, with one index taggedstaging,pr-22. - Checks.
test:scriptspasses 14/14, andapp:sync --checkpasses. - Leftover references. I searched the repo (excluding the lockfile and
documentation.json) forlatest,develop, Docker Hub,docker.ioand the baremoleculemaker/test-frontend. Nothing expectslatestor Docker Hub. The remaininglatesthits areruns-on: ubuntu-latest, commented-out nightwatch config, and the mutagenesis fixture.
Findings
- The package is private, and an anonymous pull returns 403 (inline,
docker.yml:9). This is the one to settle before #15. e2e.yml:4still triggers ondevelop. It's not in the diff, so it isn't an inline comment. It's harmless, but it's the lastdevelopreference left after this PR drops the branch fromdocker.yml.- Nit: the
MAIN_REPOkey in a test (inline).
Note: staging is whichever PR run finished last. There is no concurrency group, so two PRs building at once race for the tag, and re-running an old PR's workflow moves staging back to it. That matches the header's "most recently built PR", so it isn't a defect. Mentioning it in case "last finished" isn't what you meant.
Didn't investigate: the push to main (no run exists yet), the iBioFoundry org's package settings, and fork and Dependabot PRs. Dependabot gets a read-only token too; I didn't check whether this repo has Dependabot enabled.
| # main main (prod runs it) and sha-<short> | ||
| # The repo must live in the org its family deploys from (moleculemaker for | ||
| # alphasynthesis, iBioFoundry for alphasynbio), because GITHUB_TOKEN can only push | ||
| # packages to its own org. Packages are public, so the cluster needs no pull secret. |
There was a problem hiding this comment.
The package this run created is private, so "Packages are public, so the cluster needs no pull secret" doesn't hold yet. (Reproduced.) The docker run (39348164629) pushed pr-22 and staging, but:
gh api /orgs/moleculemaker/packages/container/test-frontendreturns"visibility": "private", linked tomoleculemaker/frontend-template;https://ghcr.io/token?scope=repository:moleculemaker/test-frontend:pullgives no anonymous token, and the manifest request for both tags returns 403.
The first push of a new package creates it private, and nothing in this workflow changes that. So every app generated from the template starts with a private image, and a cluster without a pull secret will fail with ImagePullBackOff once #15 deploys it. The org does allow public containers (job-template is public), so this looks like a one-time manual step per app (Package settings → Change visibility, done by an org admin). I haven't checked whether a package linked to an internal repo, which this one is, can be made public.
Possible fixes: document that step (README / the workflow header) and make it part of creating an app, or keep imagePullSecrets in the chart, as mutagenesis does today. Either way, the comment should match what actually happens.
There was a problem hiding this comment.
Confirmed, and it explains itself: this repo is INTERNAL, and the package inherited that as private. job-template is a PUBLIC repo and its first push created a public package. Fixed the claim in fe3d8ad: the header now says a package takes its repo's visibility on first push. New apps get public repos (as agreed), so they need no pull secret. An internal or private repo's package needs an org admin to make it public, or a pull secret.
I haven't changed the visibility of test-frontend or this repo; I'm asking the user, since it matters for the #15 canary. The existing frontends (ezspecificity, aceretro, mutagenesis) are also INTERNAL, so moving them onto this workflow would hit the same thing.
Also in fe3d8ad: e2e.yml no longer triggers on develop, and the test example's key is now IMAGE. Staging-tag race: noted and left as is. It's the known last-PR-built-wins tradeoff, and a concurrency group would only change it to last-started-wins.
| assert.equal(out, [ | ||
| 'env:', | ||
| ' MAIN_REPO: moleculemaker/ezspecificity-frontend # app:sync image', | ||
| ' MAIN_REPO: ghcr.io/moleculemaker/ezspecificity-frontend # app:sync image', |
There was a problem hiding this comment.
Nit: this managed-line example still uses MAIN_REPO, which #21 removed. It now holds an image ref, which reads like the old mis-tag. Renaming the key to something like IMAGE (here and on line 53) would avoid confusion.
Review of #22 (5579414557), reproduced: the first push created ghcr.io/moleculemaker/test-frontend as a private package because this repo is internal (job-template, a public repo, got a public package). The header claimed packages are public; it now says a package takes its repo's visibility on first push, and what an internal repo's app needs instead. Also: e2e.yml no longer triggers on the nonexistent develop branch, and the managed-line test uses a neutral key.
matthewberry
left a comment
There was a problem hiding this comment.
Re-review of fe3d8ad. Nothing blocking.
-
The private-package finding is addressed. The header now matches what I saw:
frontend-templateisINTERNAL, and its package came outprivate;job-templateisPUBLIC, and so is its package.
That's two data points, not a reading of GitHub's docs, but they agree with the stated rule. The "admin makes it public or add a pull secret" path is now written where the next app will read it. The current
test-frontendpackage is still private, which is expected until someone changes it. -
developis gone frome2e.yml, and the test key is nowIMAGE. I searched.githubfordevelop: no hits. -
Checks.
test:scriptspasses 14/14, andapp:sync --checkpasses.E2E on Chromepasses on this head.dockerwas still running when I checked. -
Staging race. Leaving it as a known tradeoff is fine.
Leftover, optional: e2e.yml still has the release trigger, which this PR removed from docker.yml. It's harmless (E2E on a release is reasonable), so only change it if you want the two workflows to match.
matthewberry
left a comment
There was a problem hiding this comment.
Review of fe3d8ad. Nothing blocking. Two new findings, both about the image the chart pulls.
Findings (inline)
- The header's "or a pull secret in the chart" fallback isn't supported by the chart (
docker.yml:14). Reproduced by reading the template. - The local
values.yamlimage has no tag, so it resolves to:latest, which this workflow no longer pushes (chart/values.yaml:7). Reproduced against the registry.
Verified
test:scriptspasses 14/14, andapp:sync --checkpasses.- The Dockerfile has no
ARG configuration, so dropping that build-arg is correct. - App ids are lowercase (
^[a-z][a-z0-9-]*[a-z0-9]$) and both family orgs are lowercase, as ghcr requires.
Didn't investigate: whether anyone installs the chart with values.yaml alone. I found no README or script that does.
| # The first push creates the package with the repo's visibility: public for a public | ||
| # repo (as new apps are, so the cluster needs no pull secret), private for an internal | ||
| # or private one. A private package needs an org admin to make it public in the | ||
| # package settings, or a pull secret in the chart, before the cluster can pull it. |
There was a problem hiding this comment.
The chart has no pull-secret support, so this fallback doesn't work. (Reproduced by reading the template.) chart/templates/deployment.yaml sets image: and imagePullPolicy: but has no imagePullSecrets, and nothing under chart/ mentions a pull secret (git grep -i pullsecret -- chart finds nothing). Putting a secret in the values would change nothing, and the pod would still hit ImagePullBackOff.
This matters for the #15 canary: frontend-template is internal and test-frontend is still private, so making the package public is currently the only option that works.
Possible fixes: add imagePullSecrets to the deployment template (mutagenesis's values already use one), or drop "or a pull secret in the chart" from the header.
There was a problem hiding this comment.
Right: deployment.yaml has no imagePullSecrets, so that clause offered something the chart can't do. I dropped it in 8aa991e; the header now says a private package must be made public. I'll correct the same option where I offered it to the user.
| annotations: {} | ||
| controller: | ||
| image: moleculemaker/test-frontend | ||
| image: ghcr.io/moleculemaker/test-frontend |
There was a problem hiding this comment.
Low: this untagged image resolves to :latest, which nothing pushes any more. (Reproduced against the registry.)
gh api /orgs/moleculemaker/packages/container/test-frontend/versionslists onlystagingandpr-22.flavor: latest=falsemeanslatestwill never appear.- Before this PR, the same untagged name resolved to Docker Hub's
moleculemaker/test-frontend:latest, which exists (updated today). imagePullPolicy: Alwaysmeans a locally built image wouldn't be used instead.
So installing the chart with only these local defaults now fails to pull. I found no README or script that does this, so it may have no users. The fix is one line in render() (app-sync.mjs:147): controller: { image: `${d.image}:main` }.
There was a problem hiding this comment.
Reproduced: an untagged image means :latest, which nothing pushes now. Fixed in 8aa991e: values.yaml renders ${image}:main, and a new test pins it. Script tests: 15/15.
… can't use Review of #22 (5579540211): - chart/values.yaml's untagged image resolved to :latest, which nothing pushes any more (reproduced: ghcr has only staging and pr-N so far). It now pins :main. - The header offered a pull secret in the chart as a fallback for private packages, but the deployment template has no imagePullSecrets, so a private package must be made public.
matthewberry
left a comment
There was a problem hiding this comment.
Re-review of 8aa991e. Nothing blocking; both findings are fixed.
- Pull secret: the header no longer offers it and says the chart has no
imagePullSecrets. A private package must be made public. - Local image:
render()andchart/values.yamlnow use:main, and a new test pins it. The tag will exist once this merges and main builds. - Checks:
test:scriptspasses 15/15, andapp:sync --checkpasses. I did not wait for CI on this head.
Closes #13.
What
docker.ymlpushes to ghcr usingGITHUB_TOKEN(permissions: packages: write). No Docker Hub secrets are needed.Tags:
pr-<N>,stagingmain(prod pins it),sha-<short>Nothing pushes
latest(flavor: latest=false). Prod still deploys straight from main, as decided. Tag rules checked against the docker/metadata-action v5 README:type=ref,event=prgivespr-2, andenable=${{ … }}is supported.app:syncderives the image asghcr.io/<org>/<id>-frontend:moleculemakerfor alphasynthesis,ibiofoundryfor alphasynbio. The chart values and the managed line indocker.ymlfollow.Removed:
releasetrigger, whose builds never pushed because neither build step's condition matched a release;developbranch trigger;configuration=build-arg, which the Dockerfile has noARGfor.Constraint
GITHUB_TOKENcan only push packages to its own org, so an app's repo must live in its family's org. That's what we agreed; the workflow header says so.Verification
test:scripts14/14. The ezspecificity and mutagenesis fixture comparisons now treat the image as the only expected difference for both families.app:sync --checkpasses.dockerrun is the first push ofghcr.io/moleculemaker/test-frontend:pr-<N>and:staging. I'll check after it runs that the package exists, is public and pulls anonymously.Not in scope
dockerstep will fail to push. Every PR so far has come from a branch in the repo.