Skip to content

Remove debugging steps TT-16060#7510

Merged
Razeen-Abdal-Rahman merged 1 commit intoreleng/release-5.8-TT-16060from
releng/releng/release-5.8-TT-16060
Nov 4, 2025
Merged

Remove debugging steps TT-16060#7510
Razeen-Abdal-Rahman merged 1 commit intoreleng/release-5.8-TT-16060from
releng/releng/release-5.8-TT-16060

Conversation

@Razeen-Abdal-Rahman
Copy link
Copy Markdown
Contributor

@Razeen-Abdal-Rahman Razeen-Abdal-Rahman commented Nov 4, 2025

PR Type

Enhancement


Description

  • Simplify container test step commands

  • Remove verbose health checks and logs

  • Slight rename of test step titles


Diagram Walkthrough

flowchart LR
  Build["Build container image"] -- "run" --> RunTest["Run container and sleep"]
  RunTest -- "execute" --> APITest["api_functionality test script"]
  APITest -- "cleanup" --> Stop["Stop test container"]
Loading

File Walkthrough

Relevant files
Enhancement
release.yml
Streamline container image test steps                                       

.github/workflows/release.yml

  • Rename test steps to include trailing period.
  • Replace multi-line run block with concise commands.
  • Remove health checks, logs, and directory listings.
  • Retain core api test and container cleanup.
+4/-55   

@Razeen-Abdal-Rahman Razeen-Abdal-Rahman requested a review from a team as a code owner November 4, 2025 23:40
@Razeen-Abdal-Rahman Razeen-Abdal-Rahman merged commit 7b8d253 into releng/release-5.8-TT-16060 Nov 4, 2025
5 checks passed
@Razeen-Abdal-Rahman Razeen-Abdal-Rahman deleted the releng/releng/release-5.8-TT-16060 branch November 4, 2025 23:40
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 4, 2025

API Changes

no api changes detected

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 4, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Flaky Timing

Replacing the readiness loop with a fixed 2s sleep may cause flaky test failures if the container takes longer to be ready; consider a minimal readiness check or longer backoff.

- name: Test the built container image with api functionality test.
  run: |
    docker run -d -p8080:8080 --name=test --platform linux/${{ matrix.arch }} --network ${{ job.container.network }} --rm test-${{ matrix.distro }}-${{ matrix.arch }}
    sleep 2
    ./ci/tests/api-functionality/api_test.sh
    sleep 2
    docker stop test || true
Inline YAML String

The second step uses a quoted multiline string for 'run', which is harder to maintain and can escape characters unexpectedly; prefer YAML block style with '|'.

    - name: Test the built container image with api functionality test.
      run: "docker run -d -p8080:8080 --name=test --platform linux/${{ matrix.arch }} --network ${{ job.container.network }} --rm test-${{ matrix.distro }}-${{ matrix.arch }}\nsleep 2\n./ci/tests/api-functionality/api_test.sh\nsleep 2\ndocker stop test || true \n"
release-tests:

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Wait for service readiness

Add a readiness loop to wait for the service to be reachable before running tests. A
fixed 2s sleep is flaky and will cause intermittent failures across environments.

.github/workflows/release.yml [511-517]

 - name: Test the built container image with api functionality test.
   run: |
     docker run -d -p8080:8080 --name=test --platform linux/${{ matrix.arch }} --network ${{ job.container.network }} --rm test-${{ matrix.distro }}-${{ matrix.arch }}
-    sleep 2
+    for i in {1..20}; do
+      if curl -sf http://localhost:8080/hello > /dev/null; then
+        break
+      fi
+      sleep 2
+      if [ $i -eq 20 ]; then
+        echo "Service did not become ready in time"
+        docker logs test || true
+        exit 1
+      fi
+    done
     ./ci/tests/api-functionality/api_test.sh
     sleep 2
     docker stop test || true
Suggestion importance[1-10]: 8

__

Why: Reintroducing a readiness loop addresses a real flakiness risk introduced by replacing a robust wait with a fixed 2s sleep; the improved_code correctly modifies the shown new hunk snippet to wait for the service before testing.

Medium
General
Add cleanup trap on exit

Ensure the container is always stopped even if the test fails by using a trap. This
prevents leaked containers and port conflicts in subsequent steps.

.github/workflows/release.yml [511-517]

 - name: Test the built container image with api functionality test.
   run: |
+    set -euo pipefail
     docker run -d -p8080:8080 --name=test --platform linux/${{ matrix.arch }} --network ${{ job.container.network }} --rm test-${{ matrix.distro }}-${{ matrix.arch }}
+    trap 'docker stop test || true' EXIT
     sleep 2
     ./ci/tests/api-functionality/api_test.sh
-    sleep 2
-    docker stop test || true
Suggestion importance[1-10]: 7

__

Why: Adding a shell trap ensures the container is stopped even on failures, improving reliability and cleanup; the proposed code aligns with the new hunk and enhances robustness without altering logic.

Medium
Use proper multiline run block

Use a multiline block for the shell script instead of a quoted single-line with
embedded newlines. This avoids YAML escaping pitfalls and preserves shell semantics
reliably.

.github/workflows/release.yml [572-573]

 - name: Test the built container image with api functionality test.
-  run: "docker run -d -p8080:8080 --name=test --platform linux/${{ matrix.arch }} --network ${{ job.container.network }} --rm test-${{ matrix.distro }}-${{ matrix.arch }}\nsleep 2\n./ci/tests/api-functionality/api_test.sh\nsleep 2\ndocker stop test || true \n"
+  run: |
+    docker run -d -p8080:8080 --name=test --platform linux/${{ matrix.arch }} --network ${{ job.container.network }} --rm test-${{ matrix.distro }}-${{ matrix.arch }}
+    sleep 2
+    ./ci/tests/api-functionality/api_test.sh
+    sleep 2
+    docker stop test || true
Suggestion importance[1-10]: 6

__

Why: Converting the quoted newline-embedded run to a YAML block scalar improves readability and reduces YAML parsing pitfalls; the improved_code accurately reflects the intended formatting change for the corresponding new hunk.

Low

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.

1 participant