Skip to content

added target config to build images#40

Merged
edinstance merged 2 commits intodevfrom
build-images-taeget-config
Oct 21, 2025
Merged

added target config to build images#40
edinstance merged 2 commits intodevfrom
build-images-taeget-config

Conversation

@edinstance
Copy link
Owner

@edinstance edinstance commented Oct 21, 2025

Summary by CodeRabbit

  • Chores
    • Updated build pipeline to add two authentication build targets (web, migrate) and remove the previous entry.
    • Build process now passes the selected target through to image creation.
    • Image tags and the "latest" tag now include the target suffix when present, including for the main branch.

@coderabbitai
Copy link

coderabbitai bot commented Oct 21, 2025

Walkthrough

Modifies the GitHub Actions build workflow to change the authentication service build matrix (adds web and migrate targets, removes an existing entry), set target from the matrix in the build-push step, and conditionally append -{target} to image push and latest tags (including main latest tag) when a target is present.

Changes

Cohort / File(s) Summary
GitHub Actions Build Workflow
.github/workflows/build_images.yml
Added two new matrix entries for the authentication service targets (web, migrate) and removed an existing authentication matrix entry; updated the build-push step to set target from matrix.target (or empty); changed image tag generation to append -{target} to both push and latest tags when target is present; main branch latest tag also includes the -{target} suffix when applicable.

Sequence Diagram(s)

sequenceDiagram
  participant GH as GitHub Actions
  participant Matrix as Matrix Runner
  participant Builder as Build/Push Step
  participant Registry as Container Registry

  Note over GH,Matrix: Workflow triggered (push/PR)
  GH->>Matrix: expand matrix (services + targets)
  Matrix->>Builder: run job with matrix.service, matrix.target
  alt target present
    Builder->>Builder: set TARGET = matrix.target
    Builder->>Registry: build image and tag as service-{{target}}:sha / service-{{target}}:latest
  else no target
    Builder->>Registry: build image and tag as service:sha / service:latest
  end
  Registry-->>Builder: push confirmation
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Autobuild docker images #36: Modifies the same GitHub Actions workflow and its image build/tagging matrix — likely directly related to these matrix and tagging changes.
  • Autobuild docker images fixes #37: Also changes .github/workflows/build_images.yml to adjust per-service matrix entries and conditional build logic, overlapping with this PR's edits.

Poem

🐇 I hopped through the matrix, two targets in sight,

web and migrate, I set them just right.
Tags now wear tails when a target's around,
Builds hum along and images are found.
A tiny rabbit cheers at CI's new sound.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "added target config to build images" directly relates to the primary changes in the changeset. The PR fundamentally adds target configuration to the build images workflow by introducing new matrix entries for targets (web and migrate) and modifying how image tags are generated based on the target parameter. The title is clear, concise, and specific enough for a teammate to understand that the change concerns target-based configuration in the build workflow, without being vague or generic.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch build-images-taeget-config

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98f5fee and 729f938.

📒 Files selected for processing (1)
  • .github/workflows/build_images.yml (2 hunks)
🔇 Additional comments (2)
.github/workflows/build_images.yml (2)

34-39: Acknowledge: keygen target issue appears resolved, but breaking change persists.

The previous review flagged a critical bug with an undefined keygen target; the current code includes only web and migrate targets, which is appropriate. However, the underlying breaking change remains: replacing the single services/authentication entry with two target-specific entries transforms the image names from ghcr.io/...-services/authentication:{ref} to ghcr.io/...-services/authentication-web:{ref} and ghcr.io/...-services/authentication-migrate:{ref}.

Any deployment configurations, infrastructure scripts, or orchestration platforms referencing the old image name will fail to pull the correct image during rollout.

Has this breaking change been communicated to teams maintaining downstream deployments? Recommend adding a migration guide or deprecation notice documenting:

  • Old image naming convention: services/authentication:{ref}
  • New image naming convention: services/authentication-{web|migrate}:{ref}
  • Timeline for decommissioning the old name (if applicable)

85-85: Verification confirmed — tag generation logic handles all matrix permutations correctly.

All verification points are satisfied:

  • Dockerfile build stages: only web and migrate targets exist (no keygen)
  • Non-targeted services: properly configured without target definitions (flights, aircraft, search, gateway)
  • Conditional logic: correctly appends -{target} suffix only when matrix.target is defined; services without targets receive empty string, producing tags without suffix
  • Latest tag: correctly limited to main branch

The workflow configuration is sound.


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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
.github/workflows/build_images.yml (1)

90-91: Image tag generation logic is correct, but formatting approach is inconsistent.

The conditional target suffix logic is correct in both lines—the target suffix is appended only when defined, and the latest tag is only applied on main branch. However, line 90 uses direct string concatenation whilst line 91 uses the format() function for the same purpose, which reduces readability and maintainability.

For consistency and clarity, consider standardising to one approach. Here is a refactored version using format() throughout:

tags: |
  ${{ format('{0}-{1}{2}:{3}', env.IMAGE_PREFIX, matrix.service, matrix.target && format('-{0}', matrix.target) || '', github.ref_name) }}
  ${{ github.ref_name == 'main' && format('{0}-{1}{2}:latest', env.IMAGE_PREFIX, matrix.service, matrix.target && format('-{0}', matrix.target) || '') || '' }}

Alternatively, if direct concatenation is preferred, simplify line 91 to match line 90's style.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a3db11 and 98f5fee.

📒 Files selected for processing (1)
  • .github/workflows/build_images.yml (2 hunks)
🔇 Additional comments (1)
.github/workflows/build_images.yml (1)

88-88: Verify that undefined matrix variables are handled correctly with empty string target.

When services without a target matrix entry (e.g., flights, aircraft, search, gateway) reach the build step, matrix.target is undefined. The expression ${{ matrix.target || '' }} should provide an empty string fallback, but this depends on how GitHub Actions handles undefined matrix variables versus falsy values.

The target parameter is supported by docker/build-push-action@v6, but it is unclear whether passing an empty string for target will skip target selection or cause unexpected behaviour. Before merging, verify this works correctly by:

  • Running a test workflow that builds a service without a target defined in the matrix (e.g., services/flights)
  • Confirming the resulting image is pushed successfully without target-specific suffixes

Alternatively, consider explicitly excluding or conditionally applying the target only when defined:

target: ${{ matrix.target || null }}

However, this may fail if docker/build-push-action v6 does not accept null values for the target parameter.

Also applies to: 90-90, 91-91

@edinstance edinstance merged commit 8b60b1b into dev Oct 21, 2025
2 checks passed
@edinstance edinstance deleted the build-images-taeget-config branch October 21, 2025 11:42
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.

1 participant