Skip to content

Conversation

@gontzess
Copy link
Contributor

@gontzess gontzess commented Dec 18, 2025

always build and push docker images to the conductorone public ECR (https://gallery.ecr.aws/conductorone) so that customers can access images without requiring auth creds. note this doesn't remove steps for pushing images to GHCR, which could be removed at a future point in time.

testing:

## confirm tags exist
aws ecr-public describe-images \
  --region us-east-1 \
  --repository-name "baton-github-test" \
  --query 'sort_by(imageDetails,& imagePushedAt)[-10:].{pushed:imagePushedAt,tags:imageTags,digest:imageDigest}' \
  --output table

aws ecr-public describe-images \
  --region us-east-1 \
  --repository-name "baton-github-test" \
  --image-ids imageTag="0.1.97" \
  --query 'imageDetails[0].{digest:imageDigest,tags:imageTags,manifestMediaType:imageManifestMediaType,artifactMediaType:artifactMediaType,pushed:imagePushedAt}' \
  --output json

## inspect ECR Public manifests, see `"mediaType": "application/vnd.oci.image.index.v1+json"`
crane manifest public.ecr.aws/conductorone/baton-github-test:0.1.97 | jq

## list the platforms - see note below
crane manifest public.ecr.aws/conductorone/baton-github-test:0.1.97 \
  | jq -r '.manifests[] | "\(.platform.os)/\(.platform.architecture)\t\(.digest)"'

note: current implementations includes buildx-generated attestation-manifest entries (SPDX + SLSA v0.2) for the images, so the cmd above lists two "unknown" os/arch and digests representing the attestation manifests for each platform digest. in a follow up PR we'll replace with cosign attest (SPDX + SLSA v1).

successful release runs:
https://github.com/ConductorOne/baton-github-test/actions/runs/20347953676
https://gallery.ecr.aws/conductorone/baton-github-test

@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

Walkthrough

Documentation comments updated in two proto files to clarify that the images field keys represent registry identifiers (e.g., "ghcr", "ecrPublic") rather than platform identifiers (e.g., "linux-amd64"). No functional changes.

Changes

Cohort / File(s) Summary
Proto documentation updates
proto/artifacts/v1/manifest.proto, proto/artifacts/v1/stable.proto
Updated doc comments for the images field to clarify that map keys are registry identifiers (e.g., "ghcr", "ecrPublic") instead of platform identifiers (e.g., "index", "linux-amd64").

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested reviewers

  • ggreer
  • laurenleach
  • kans

Poem

🐰 A registry, not a platform, we now say,
Comments bloom clearer every day,
"ghcr" and "ecrPublic" take their rightful stage,
Documentation shines—the wisdom of the age! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title describes pushing Docker images to public ECR, which is the core objective of the PR. However, the actual code changes only update documentation comments in proto files about registry identifiers versus platform identifiers—no actual implementation changes to push images. The title is misleading relative to the actual changeset. Update the title to reflect the actual changes, such as 'Update manifest documentation to reflect registry-based image keys' or clarify whether the PR includes implementation changes beyond documentation updates.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch gontzess/push-dockers-to-ecr-1

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 084349e and caee5ac.

⛔ Files ignored due to path filters (6)
  • .github/workflows/release.yaml is excluded by none and included by none
  • cmd/extract-images/main.go is excluded by none and included by none
  • pb/artifacts/v1/manifest.pb.go is excluded by !**/*.pb.go, !pb/** and included by none
  • pb/artifacts/v1/stable.pb.go is excluded by !**/*.pb.go, !pb/** and included by none
  • templates/.goreleaser-docker-lambda-template.yaml is excluded by none and included by none
  • templates/.goreleaser-docker-oci-template.yaml is excluded by none and included by none
📒 Files selected for processing (2)
  • proto/artifacts/v1/manifest.proto (1 hunks)
  • proto/artifacts/v1/stable.proto (1 hunks)
🔇 Additional comments (2)
proto/artifacts/v1/manifest.proto (1)

35-36: Documentation clarification looks good.

The updated comment accurately describes the images field as using registry identifiers (e.g., "ghcr", "ecrPublic") rather than platform identifiers. This aligns well with the PR's objective to support multiple container registries.

Note: The PR objectives describe workflow changes to push Docker images to public ECR, but only proto documentation changes are provided for review. Please verify that the corresponding workflow implementation changes exist and correctly use these registry identifiers as map keys.

proto/artifacts/v1/stable.proto (1)

37-38: Documentation update is consistent and correct.

This change mirrors the update in manifest.proto and maintains consistency across both proto files. The registry identifier semantics are appropriate for the Stable message's images field.


Comment @coderabbitai help to get the list of available commands and usage tips.

GITHUB_TOKEN: ${{ secrets.RELENG_GITHUB_TOKEN }}
COSIGN_EXPERIMENTAL: "1"

- name: Configure Lambda ECR AWS credentials via OIDC
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the future i may try to separate out the lambda vs oci into two different gh jobs, however last time i tried there were a bunch of issues with conditional logic for subsequent jobs running if no fails

env:
REPO_NAME: ${{ github.event.repository.name }}
DOCKERFILE_PATH: ../_workflows/Dockerfile
DIST_DIR: dist/oci
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have to build the artifacts in different (sub)directories to avoid issues with overwriting/cleaning assets that we need at the end for extracting digests. chose sub directories since dist/ is already in gitignore for the callers

envsubst '$REPO_NAME' < _workflows/templates/.Dockerfile-lambda-template > _workflows/Dockerfile.lambda
echo "Generated Dockerfile.lambda:"
cat _workflows/Dockerfile.lambda
echo "Generated Dockerfile.lambda:"; cat _workflows/Dockerfile.lambda

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe printf this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i actually change this in the next PR to use envsubst and tee: https://github.com/ConductorOne/github-workflows/pull/39/changes#diff-e426ed45842837026e10e66af23d9c7077e89eacbe6958ce7cb991130ad05adaR354

lmk if you have thoughts on a better way

@gontzess gontzess merged commit 9237ef3 into main Dec 23, 2025
1 check passed
@gontzess gontzess deleted the gontzess/push-dockers-to-ecr-1 branch December 23, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants