Skip to content

tests: define /tmp/k8s-service-logs in a single place only.#17184

Merged
porridge merged 1 commit intomasterfrom
porridge/k8s-service-logs
Oct 14, 2025
Merged

tests: define /tmp/k8s-service-logs in a single place only.#17184
porridge merged 1 commit intomasterfrom
porridge/k8s-service-logs

Conversation

@porridge
Copy link
Copy Markdown
Contributor

@porridge porridge commented Oct 8, 2025

Description

Backround: I'd like us to collect additional OLM artifacts in case of failed operator installation/upgrade.

However knowing where to drop them off for gathering into the additional stackrox e2e artifacts bucket is not so simple.
This is because we sometimes collect to /tmp/k8s-service-logs and sometimes to a more elaborate path like .../part-1/k8s-logs. There probably are other paths too. And these paths are sprinkled as magic strings all over the codebase.

Before:

[stackrox]$ git grep -c k8s-service-logs
.openshift-ci/post_tests.py:1
operator/tests/TROUBLESHOOTING_E2E_TESTS.md:1
scripts/ci/collect-collector-metrics.sh:1
scripts/ci/collect-infrastructure-logs.sh:2
scripts/ci/collect-qa-service-logs.sh:1
scripts/ci/collect-service-logs.sh:2
scripts/secured-cluster-diagnostics.sh:1
[stackrox]$ 

While providing some convenience, the practice of repeating such path in multiple places makes it really hard to evolve the codebase. This is a baby step towards taking this under control.

After:

[stackrox]$ git grep k8s-service-logs
.openshift-ci/post_tests.py:    K8S_LOG_DIR = "/tmp/k8s-service-logs"
operator/tests/TROUBLESHOOTING_E2E_TESTS.md:... then follow the path `k8s-service-logs/stackrox-operator/pods` ...
[stackrox]$ 

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • no tests changed

How I validated my change

I carefully looked at where and how each script is being invoked and made sure no call omits the now-mandatory arguments. Then I had claude double check.

My prompt:

> Have a look at the last Commit.
  Check that it is sound and that the author did not miss any place which required changes for correctness reasons.
  In particular, pay close attention to cases where the scripts that now require a directory ath argument are called in a way which does not provide it.
  If you find such places make necessary connections but do not commit.

It said:

● Analysis Summary

  I've thoroughly examined the last commit b19941aa51 and all the places where the modified scripts are called throughout the codebase. The commit is sound and complete.

  Scripts that now require directory path arguments:
  - collect-infrastructure-logs.sh - requires <output-dir>
  - collect-service-logs.sh - requires <namespace> and <output-dir>
  - collect-qa-service-logs.sh - requires DIR
  - collect-collector-metrics.sh - requires <namespace> and <output-dir>
  - secured-cluster-diagnostics.sh - requires <namespace> and <output-dir>

  All existing calls verified:
  1. .openshift-ci/post_tests.py:209,319,227 - ✅ All provide required arguments
  2. tests/common.go:828 - ✅ Provides both namespace and directory
  3. tests/e2e/run-scanner-v4-install.bats:421,427 - ✅ Provides both arguments
  4. qa-tests-backend/src/main/groovy/util/Helpers.groovy:124,125,126 - ✅ All provide required arguments
  5. tests/e2e/lib.sh:974 - ✅ Provides both arguments

  No issues found - all existing calls to these scripts already provide the required directory path arguments. The author correctly updated all necessary places in the codebase when making the scripts require explicit directory arguments.

I also had a look at the logs from all triggered CI jobs, and there were no unexpected occurrences of Usage: ... error messages.

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 8, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Oct 8, 2025

/test all

@rhacs-bot
Copy link
Copy Markdown
Contributor

Images are ready for the commit at b19941a.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.9.x-1005-gb19941aa51.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.82%. Comparing base (478d28a) to head (b19941a).
⚠️ Report is 64 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #17184      +/-   ##
==========================================
+ Coverage   48.80%   48.82%   +0.01%     
==========================================
  Files        2714     2715       +1     
  Lines      203120   203156      +36     
==========================================
+ Hits        99138    99189      +51     
+ Misses      96159    96151       -8     
+ Partials     7823     7816       -7     
Flag Coverage Δ
go-unit-tests 48.82% <ø> (+0.01%) ⬆️

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

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@porridge porridge added the auto-retest PRs with this label will be automatically retested if prow checks fails label Oct 9, 2025
@porridge
Copy link
Copy Markdown
Contributor Author

porridge commented Oct 9, 2025

/retest

@red-hat-konflux

This comment was marked as off-topic.

@porridge porridge requested a review from a team October 9, 2025 08:31
@porridge porridge marked this pull request as ready for review October 9, 2025 08:31
@rhacs-bot
Copy link
Copy Markdown
Contributor

/retest

@porridge porridge merged commit fe7cfce into master Oct 14, 2025
110 checks passed
@porridge porridge deleted the porridge/k8s-service-logs branch October 14, 2025 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-retest PRs with this label will be automatically retested if prow checks fails

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants