Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions .github/workflows/e2e-caas-full-install.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,22 +56,30 @@ jobs:
- '!**/*.md'
- '!**/docs/**'

# Builds both fulfillment-service and osac-operator from this PR's current
# code and installs them together in one cluster -- not one "component
# under test" plus a stale pinned image for the other. Runs for any change
# touching either component (see `changes` above) rather than being
# path-scoped per component; splitting this by component to avoid running
# both suites when only one changed is deferred to the future full
# path-based CI matrix, once its skip-logic pattern is verified.
# Builds fulfillment-service, osac-operator, and bare-metal-fulfillment-operator
# from this PR's current code and installs them together in one cluster --
# not one "component under test" plus a stale pinned image for the others.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
# Runs for any change touching any component (see `changes` above) rather
# than being path-scoped per component; splitting this by component to
# avoid running both suites when only one changed is deferred to the
# future full path-based CI matrix, once its skip-logic pattern is
# verified. (osac-aap is not built here -- that's a pre-existing scope
# decision for this flow, unrelated to BMF, not changed by this entry.)
#
# bare-metal-fulfillment-operator's image override (bmf.image.repository)
# is currently a no-op in this flow specifically: values/caas-ci/values.yaml
# sets bmf.enabled: false, so the chart isn't deployed here at all. Still
# built from source for consistency with the other components -- revisit
# if caas ever needs BMF enabled.
e2e-caas-full-install:
needs: changes
if: needs.changes.outputs.should-run == 'true'
uses: osac-project/osac-test-infra/.github/workflows/e2e-caas-full-install.yml@main
with:
test-suite: ${{ inputs.test-suite || 'caas' }}
test-filter: ${{ inputs.test-filter || '' }}
# Build both component images from the PR's fork/branch (same monorepo checkout)
components: '[{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"fulfillment-service/Containerfile","imageKey":"service.images.service"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"osac-operator/Containerfile","imageKey":"operator.image.repository"}]'
# Build all component images from the PR's fork/branch (same monorepo checkout)
components: '[{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"fulfillment-service/Containerfile","imageKey":"service.images.service"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"osac-operator/Containerfile","imageKey":"operator.image.repository"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"bare-metal-fulfillment-operator/Containerfile","imageKey":"bmf.image.repository"}]'
# Pass author_association for fork PR authorization (empty for non-fork PRs)
fork-pr-author-association: ${{ github.event.pull_request.head.repo.fork && github.event.pull_request.author_association || '' }}
fork-pr-author: ${{ github.event.pull_request.head.repo.fork && github.event.pull_request.user.login || '' }}
Expand Down
25 changes: 16 additions & 9 deletions .github/workflows/e2e-vmaas-full-install.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,21 @@ jobs:
- '!**/*.md'
- '!**/docs/**'

# Builds fulfillment-service, osac-operator, and osac-aap's execution
# environment from this PR's current code and installs them together in
# one cluster -- not one "component under test" plus stale pinned images
# for the others. Runs for any change touching any component (see
# `changes` above) rather than being path-scoped per component;
# splitting this by component to avoid running both suites when only one
# changed is deferred to the future full path-based CI matrix, once its
# skip-logic pattern is verified.
# Builds fulfillment-service, osac-operator, osac-aap's execution
# environment, and bare-metal-fulfillment-operator from this PR's current
# code and installs them together in one cluster -- not one "component
# under test" plus stale pinned images for the others. Runs for any
# change touching any component (see `changes` above) rather than being
# path-scoped per component; splitting this by component to avoid
# running both suites when only one changed is deferred to the future
# full path-based CI matrix, once its skip-logic pattern is verified.
#
# bare-metal-fulfillment-operator's image override (bmf.image.repository)
# is currently a no-op in this flow specifically: values/vmaas-ci/values.yaml
# sets bmf.enabled: false, so the chart isn't deployed here at all. Still
# built from source for consistency with the other 3 components (all 4
# should build from the PR in every e2e flow, even where one isn't
# currently deployed) -- revisit if vmaas ever needs BMF enabled.
Comment on lines +71 to +73

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Limit the “every e2e flow” claim.

.github/workflows/e2e-caas-full-install.yml Lines 66-67 explicitly exclude osac-aap. Therefore, all four components do not build in every e2e flow. Scope this statement to the VMaaS flow or remove the cross-flow claim.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/e2e-vmaas-full-install.yml around lines 71 - 73, Update
the comment near the VMaaS component build configuration to remove or narrow the
claim that all four components build in every e2e flow; describe the expectation
only for this VMaaS flow while preserving the note about revisiting BMF support.

e2e-vmaas-full-install:
needs: changes
if: needs.changes.outputs.should-run == 'true'
Expand All @@ -72,7 +79,7 @@ jobs:
test-suite: ${{ inputs.test-suite || 'vmaas' }}
test-filter: ${{ inputs.test-filter || '' }}
# Build all component images from the PR's fork/branch (same monorepo checkout)
components: '[{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"fulfillment-service/Containerfile","imageKey":"service.images.service"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"osac-operator/Containerfile","imageKey":"operator.image.repository"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"osac-aap/execution-environment/execution-environment.yaml","imageKey":"aap.bootstrap.image","buildType":"ansible-builder"}]'
components: '[{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"fulfillment-service/Containerfile","imageKey":"service.images.service"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"osac-operator/Containerfile","imageKey":"operator.image.repository"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"osac-aap/execution-environment/execution-environment.yaml","imageKey":"aap.bootstrap.image","buildType":"ansible-builder"},{"repo":"${{ github.event.pull_request.head.repo.full_name || github.repository }}","ref":"${{ github.event.pull_request.head.ref || github.ref_name }}","containerfile":"bare-metal-fulfillment-operator/Containerfile","imageKey":"bmf.image.repository"}]'
# Pass author_association for fork PR authorization (empty for non-fork PRs)
fork-pr-author-association: ${{ github.event.pull_request.head.repo.fork && github.event.pull_request.author_association || '' }}
fork-pr-author: ${{ github.event.pull_request.head.repo.fork && github.event.pull_request.user.login || '' }}
Expand Down
Loading