CNTRLPLANE-3642: ci: add top-level directory drift detection - #8773
CNTRLPLANE-3642: ci: add top-level directory drift detection#8773mgencur wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mgencur: This pull request references CNTRLPLANE-3642 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new CI verification mechanism is added to enforce that 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/ci-tooling
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8773 +/- ##
==========================================
+ Coverage 41.84% 41.88% +0.04%
==========================================
Files 759 759
Lines 94073 94155 +82
==========================================
+ Hits 39361 39434 +73
- Misses 51956 51961 +5
- Partials 2756 2760 +4 see 8 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,34 @@ | |||
| api | |||
There was a problem hiding this comment.
The file is a plain list with no context. Someone encountering it cold won't know what it's for, why it exists, or what to do when they add a directory. A two-line comment at the top would save confusion:
# Tracked list of top-level directories in this repo.
# Used by hack/ci/verify-toplevel-dirs.sh to detect drift in the CI skip pattern.
# When adding a new top-level directory, update this file AND the pipeline_skip_if_only_changed
# regex in ci-operator (unless the new directory contains E2E tests — see CNTRLPLANE-3642).
There was a problem hiding this comment.
This file is read by a script so I can't easily add the description. Also, next to this file is verify-toplevel-dirs.sh which has this text:
# Verifies that the tracked top-level directory list stays in sync with
# the actual git-tracked directories. This prevents the CI skip pattern
# (pipeline_skip_if_only_changed) from silently going stale when new
# top-level directories are added to the repo.
Isn't it enough? I could possibly add a README.txt to this folder and mention this file but since it's already mentioned in the script that is named very similarly, I thought this would be enough. Please let me know.
There was a problem hiding this comment.
Oh sorry, I missed it. I guess it would be fine then :)
| support | ||
| sync-fg-configmap | ||
| sync-global-pullsecret | ||
| test |
There was a problem hiding this comment.
test/ is in toplevel-dirs.txt — but NOT in the CI skip pattern, and the script doesn't enforce that distinction:
hack/ci/toplevel-dirs.txt line 32 includes test. The verify script checks that the tracked list matches actual top-level directories — that's correct. But the pipeline_skip_if_only_changed regex in PR #80732 intentionally excludes test/ because it contains E2E tests.
This means:
- Someone adds a new top-level directory → verify fails → they update toplevel-dirs.txt ✅
- They are then also told (by the error message on line 29) to update the CI operator regex ✅
- But the script has no way to distinguish "this dir should be in the regex" from "this dir is intentionally excluded from the regex (like test/)"
The error message on line 29 says:
"Also update the pipeline_skip_if_only_changed regex in the ci-operator config."
This is misleading for test/ — if someone adds a new directory that also contains E2E tests (like a hypothetical test-extended/), they should NOT add it to the skip regex, but the error message implies they should. There's no documentation of the exclusion logic.
Suggested fix: Add a comment in toplevel-dirs.txt above test (or in a separate excluded-from-skip.txt section) and update the error message to reference the distinction explicitly:
# Directories listed here are tracked top-level dirs.
# NOT all of them belong in the pipeline_skip_if_only_changed regex.
# Dirs containing E2E tests (e.g. test/) must be excluded from the regex.
# See hack/ci/verify-toplevel-dirs.sh and CNTRLPLANE-3642 for context.
There was a problem hiding this comment.
OK. Let me update the text.
Add a verification script and tracked directory list that ensures the CI skip pattern (pipeline_skip_if_only_changed) stays in sync when new top-level directories are added to the repo. The check runs as part of `make verify-parallel`. - hack/ci/toplevel-dirs.txt: canonical list of top-level dirs - hack/ci/verify-toplevel-dirs.sh: diffs actual vs tracked dirs - Makefile: wire verify-toplevel-dirs into verify-parallel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8b94ccf to
2e1e122
Compare
|
@mgencur: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is not a test execution failure — no Prow CI test actually ran and failed. The Root CauseThe tide ERROR is caused by multiple compounding issues, all of which must be resolved before the PR can be merged:
The tide configuration from
Recommendations
Evidence
|
|
Closing. This is too complicated. There will be a simpler regex that doesn't need updates, in openshift/release#80732 |
3 participants
What this PR does / why we need it:
Add a verification script and tracked directory list that ensures the CI skip pattern (pipeline_skip_if_only_changed) stays in sync when new top-level directories are added to the repo. The check runs as part of
make verify-parallel.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3642
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit