[DAPS-1906-1] - feature: add core unit testing to CI#1907
[DAPS-1906-1] - feature: add core unit testing to CI#1907JoshuaSBrown merged 7 commits intodevelfrom
Conversation
Reviewer's GuideAdds a new GitLab CI unit test job for the core component and updates the core Docker build to compile and include test binaries in the intermediate image so they can be executed in CI, without changing the final production image contents. Sequence diagram for the new core unit CI jobsequenceDiagram
participant GitLabCI
participant GitLabRunner
participant DockerRegistry
participant CoreUnitContainer
GitLabCI->>GitLabRunner: Schedule run-core-unit-job
GitLabRunner->>DockerRegistry: docker login REGISTRY with HARBOR_USER and token
GitLabRunner->>DockerRegistry: Pull core intermediate image
DockerRegistry-->>GitLabRunner: core intermediate image
GitLabRunner->>GitLabRunner: Generate random container suffix
GitLabRunner->>CoreUnitContainer: docker run --entrypoint bash ... ctest
CoreUnitContainer->>CoreUnitContainer: ctest --test-dir build -LE integration|fixture
CoreUnitContainer-->>GitLabRunner: Test results and exit code
GitLabRunner-->>GitLabCI: Report job status based on ctest result
Flow diagram for Docker multi-stage build with enabled testsflowchart TD
subgraph Build_Stage[Docker build stage: core build]
direction TB
S1[Copy source and scripts into BUILD_DIR]
S2[Run generate_dependencies_config.sh]
S3[Run generate_datafed.sh]
S4[Configure CMake<br/>-DBUILD_TESTS=True?<br/>-DENABLE_UNIT_TESTS=True<br/>-DENABLE_INTEGRATION_TESTS=True]
S5[cmake --build build -j 8]
S6[cmake --build build --target install]
S7[Intermediate image contains server and test binaries]
S1 --> S2 --> S3 --> S4 --> S5 --> S6 --> S7
end
subgraph Prod_Stage[Docker runtime stage: core]
direction TB
P1[FROM RUNTIME AS core]
P2[Copy only server binary from build stage]
P3[Final production image without test binaries]
P1 --> P2 --> P3
end
S7 -->|copy server only| P2
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
run-core-unit-job, consider usingCI_COMMIT_REF_SLUGinstead of manually lowercasingCI_COMMIT_REF_NAMEto avoid issues with branch names that contain unsupported characters in Docker container names/tags. - The Dockerfile now enables both unit and integration tests (
-DENABLE_UNIT_TESTS=Trueand-DENABLE_INTEGRATION_TESTS=True), but the CI job filters out integration tests (-LE "integration|fixture"); if integration tests are not needed for this stage, you might want to leave them disabled to reduce build time and image size.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run-core-unit-job`, consider using `CI_COMMIT_REF_SLUG` instead of manually lowercasing `CI_COMMIT_REF_NAME` to avoid issues with branch names that contain unsupported characters in Docker container names/tags.
- The Dockerfile now enables both unit and integration tests (`-DENABLE_UNIT_TESTS=True` and `-DENABLE_INTEGRATION_TESTS=True`), but the CI job filters out integration tests (`-LE "integration|fixture"`); if integration tests are not needed for this stage, you might want to leave them disabled to reduce build time and image size.
## Individual Comments
### Comment 1
<location path=".gitlab/stage_unit.yml" line_range="144-150" />
<code_context>
+ - ci-datafed-core
+ - docker
+ script:
+ - BRANCH_LOWER=$(echo "$CI_COMMIT_REF_NAME" | tr '[:upper:]' '[:lower:]')
+ - docker login "${REGISTRY}" -u "${HARBOR_USER}" -p "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}"
+ - ./scripts/container_stop.sh -n "core-unit-" -p
+ - random_string=$(bash -c "cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 10 | head -n 1")
+ - |
+ docker run --rm \
+ --name "core-unit-${BRANCH_LOWER}-${CI_COMMIT_SHORT_SHA}-${random_string}" \
+ --entrypoint bash \
+ -t "${REGISTRY}/${PROJECT}/${COMPONENT}-${INTERMEDIATE_LAYER_NAME}-${BRANCH_LOWER}:${CI_COMMIT_SHA}" \
</code_context>
<issue_to_address>
**issue:** Sanitize branch name to avoid invalid characters in Docker image/container names.
`CI_COMMIT_REF_NAME` may contain characters invalid in Docker names (e.g. `/`, `@`, spaces in `feature/foo`, `release@candidate`). Please further sanitize `BRANCH_LOWER` (e.g. replace invalid chars with `-` and optionally truncate to a safe length) before using it in `--name` and the image path so this works for all branch names.
</issue_to_address>
### Comment 2
<location path=".gitlab/stage_unit.yml" line_range="145" />
<code_context>
+ - docker
+ script:
+ - BRANCH_LOWER=$(echo "$CI_COMMIT_REF_NAME" | tr '[:upper:]' '[:lower:]')
+ - docker login "${REGISTRY}" -u "${HARBOR_USER}" -p "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}"
+ - ./scripts/container_stop.sh -n "core-unit-" -p
+ - random_string=$(bash -c "cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 10 | head -n 1")
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Use `--password-stdin` for Docker login to avoid exposing credentials in process args.
Passing the token via `-p` exposes it in process arguments, which may be visible in process lists or logs. Prefer piping the token into `docker login` with `--password-stdin`, e.g.:
```bash
- docker login "${REGISTRY}" -u "${HARBOR_USER}" -p "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}"
+ echo "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}" | docker login "${REGISTRY}" -u "${HARBOR_USER}" --password-stdin
```
```suggestion
- echo "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}" | docker login "${REGISTRY}" -u "${HARBOR_USER}" --password-stdin
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - BRANCH_LOWER=$(echo "$CI_COMMIT_REF_NAME" | tr '[:upper:]' '[:lower:]') | ||
| - docker login "${REGISTRY}" -u "${HARBOR_USER}" -p "${HARBOR_DATAFED_GITLAB_CI_REGISTRY_TOKEN}" | ||
| - ./scripts/container_stop.sh -n "core-unit-" -p | ||
| - random_string=$(bash -c "cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 10 | head -n 1") | ||
| - | | ||
| docker run --rm \ | ||
| --name "core-unit-${BRANCH_LOWER}-${CI_COMMIT_SHORT_SHA}-${random_string}" \ |
There was a problem hiding this comment.
issue: Sanitize branch name to avoid invalid characters in Docker image/container names.
CI_COMMIT_REF_NAME may contain characters invalid in Docker names (e.g. /, @, spaces in feature/foo, release@candidate). Please further sanitize BRANCH_LOWER (e.g. replace invalid chars with - and optionally truncate to a safe length) before using it in --name and the image path so this works for all branch names.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Description
Part 1 of 2 in supporting testing of Core Server.
Summary by Sourcery
Add CI support for running core component unit tests using an intermediate Docker image that includes test binaries.
Build:
CI:
Tests: