Skip to content

Pre-build ldap image for integration test#92

Open
lubomir wants to merge 1 commit into
mainfrom
overseer/89
Open

Pre-build ldap image for integration test#92
lubomir wants to merge 1 commit into
mainfrom
overseer/89

Conversation

@lubomir

@lubomir lubomir commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🤖 This was posted automatically by an AI agent.

Summary

Pre-builds the LDAP server used by the integration test as a dedicated container
image instead of performing a live pip install ldaptor twisted on every test run.

Changes

New files

  • integration-tests/images/ldap-server/server.py – the existing in-memory
    LDAP server script (extracted verbatim from the inline ConfigMap in the
    integration-test pipeline). Listens on port 1389 as an arbitrary UID,
    compatible with OpenShift's restricted-v2 SCC.

  • integration-tests/images/ldap-server/Containerfile – builds a
    fedora-minimal:40-based image that installs ldaptor and twisted via
    pip3 and sets CMD ["python3", "/app/server.py"].

  • .tekton/ldap-server-pull-request.yaml – Konflux build PipelineRun for
    pull requests. Triggers only when files under
    integration-tests/images/ldap-server/ change (pathChanged() CEL filter).
    Uses service account build-pipeline-ldap-server and sets image-expires-after: 5d.

  • .tekton/ldap-server-push.yaml – Konflux build PipelineRun for pushes to
    main. Same path filter. Applies the main tag via the apply-tags task.

Modified file

  • .tekton/integration-test-eaas.yaml:
    • parse-snapshot: adds a ldap-server-image-url result that extracts the
      ldap-server component's containerImage from the Snapshot JSON.
    • deploy-openldap: replaces the inline ConfigMap creation, pip install,
      volume mount, and volumes with a single Deployment that references the
      pre-built image passed as the ldap-server-image parameter.

Notes

The build-pipeline-ldap-server service account must be provisioned in the
team-sp-rhelcmp-tenant namespace before the new pipelines can run; this is
an operator/platform concern outside the scope of this PR.

@codecov-commenter

codecov-commenter commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.69%. Comparing base (37bad18) to head (a22ae0f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #92   +/-   ##
=======================================
  Coverage   83.69%   83.69%           
=======================================
  Files          13       13           
  Lines        1325     1325           
=======================================
  Hits         1109     1109           
  Misses        216      216           
Flag Coverage Δ
unit-tests 83.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lubomir

lubomir commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

The plan made some incorrect assumptions about how the tests are going to run. Apparently when more than one component is built in a PR, each one gets a separate test pipeline, plus there is one "group" test pipeline.

In each case the ITS is supposed to get a snapshot with all components. For pipelines running on a single component, the other components are supposed to be populated from latest stable build.

In this case, that is not working as there is no stable build for the ldap-server component.

We get:

  • group: both images present from this PR, tests pass
  • just ldap-server: snapshot uses cts from latest main commit and tests work
  • just cts: snapshot doesn't have ldap-server, and the tests fail

The fallback in the ldap-server extraction is not helpful. If there was something to fall back to, it would have been included in the Snapshot in the first place. If the snapshot doesn't contain ldap-server, the test should just be skipped.

This is only an issue for the initial PR. Once there is an ldap-server built from main branch, the snapshot should always have some url available.

Also, the new build pipelines should be updated to match what Konflux onboarding proposed in konflux-ldap-server branch.

@lubomir

lubomir commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 This was posted automatically by an AI agent.

Two issues addressed:

Integration test skip when ldap-server is absent:

  • parse-snapshot now outputs a ldap-server-present result (true/false) instead of falling back to the :main tag when the component is missing from the Snapshot.
  • All deployment and test tasks (provision-environment through run-tests) are gated with a when: ldap-server-present == "true" condition.
  • A finally task (report-result) always runs and emits skipped when ldap-server-present is false, or the actual test result otherwise. TEST_OUTPUT points to this finally task.

Build pipeline alignment with konflux-ldap-server branch:

  • Added pipelinesascode.tekton.dev/cancel-in-progress: "true" to the PR pipeline and "false" to the push pipeline.
  • Expanded the on-cel-expression trigger to also fire when .tekton/ldap-server-pull-request.yaml / .tekton/ldap-server-push.yaml or a Containerfile changes, matching the Konflux onboarding proposal.
  • Removed the spurious creationTimestamp: field from both pipeline metadata sections.

@lubomir

lubomir commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 This was posted automatically by an AI agent.

Code Review

All 9 Acceptance Criteria are satisfied. The implementation correctly addresses the reviewer's feedback about skip-on-absence and alignment with the konflux-ldap-server reference branch.

Findings

NIT — "Containerfile".pathChanged() filter matches no real file (introduced by this PR, same as reference branch)

In both .tekton/ldap-server-pull-request.yaml and .tekton/ldap-server-push.yaml, the CEL trigger includes:

|| "Containerfile".pathChanged()

In Pipelines As Code, a bare "Containerfile" path matches only a file named Containerfile at the repository root. The actual file is at integration-tests/images/ldap-server/Containerfile, which is already covered by the "integration-tests/images/ldap-server/***".pathChanged() glob. The bare filter is therefore a dead clause — it never fires independently, nor does it cause spurious triggers (there is no root Containerfile). This was copied verbatim from the konflux-ldap-server reference branch, so it is consistent with the Konflux onboarding proposal. No functional impact.

NIT — Push pipeline missing enable-cache-proxy param (introduced by this PR, diverges from reference branch)

.tekton/ldap-server-push.yaml does not declare an enable-cache-proxy pipeline parameter and does not pass it to the init task, while .tekton/ldap-server-pull-request.yaml and the konflux-ldap-server reference branch for the push pipeline both do. The init task defaults this to "false", so cache proxy is simply always disabled for push builds. This is functionally acceptable but is an unintentional divergence from the reference branch.


No MUST FIX issues found. The skip-on-absence logic (using ldap-server-present result and when conditions across all deployment/test tasks, plus the finally report-result task) is implemented correctly. The default: "" on the test-result param in report-result correctly handles the case where run-tests was skipped and produced no result. The Containerfile and server.py are well-structured and functionally correct.

@lubomir

lubomir commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

The only difference to the reference pipeline on konflux-ldap-server branch must be the CEL expression.

Add a Containerfile and server.py for the LDAP server image, build
pipelines for Konflux (pull-request and push), and update the
integration-test pipeline to consume the pre-built image from the
Snapshot rather than installing ldaptor/twisted at test time.

When the Snapshot does not contain the ldap-server component (e.g. on
initial PRs before any push build exists), the integration test is
skipped rather than failing.

Generated-By: OpenCode (google-vertex-anthropic/claude-sonnet-4-6@default)
@lubomir

lubomir commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 This was posted automatically by an AI agent.

Both build pipeline files (ldap-server-pull-request.yaml and ldap-server-push.yaml) are now taken verbatim from the konflux-ldap-server reference branch. The CEL expressions are unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants