Skip to content

fix: handle repos with different release branch naming in crd-schema-checker#1897

Merged
istio-testing merged 2 commits into
istio-ecosystem:mainfrom
fjglira:fix-linter-midstream
Apr 30, 2026
Merged

fix: handle repos with different release branch naming in crd-schema-checker#1897
istio-testing merged 2 commits into
istio-ecosystem:mainfrom
fjglira:fix-linter-midstream

Conversation

@fjglira
Copy link
Copy Markdown
Contributor

@fjglira fjglira commented Apr 29, 2026

We are getting failures in a fork after the changes were made to the linter on this PR

Issue

When not on a release branch, the script previously derived the comparison branch from PREVIOUS_VERSION (e.g. release-1.29). This broke in repos that use a different naming convention (e.g. forks of repos using release-3.x), and also meant the pinned value could go stale as new releases were cut.

This change aligns the non-release-branch case with the release-branch case, which already auto-detects the relevant branch via sort -V. When not on a release branch, the script now always uses the latest available release-* branch. A PREVIOUS_BRANCH env var is also added as an explicit override when needed.

Behavior by scenario

Scenario Before After
Upstream, on release-1.30 auto-detects release-1.29 unchanged ✓
Upstream, on main uses release-1.29 (pinned, potentially stale) auto-detects release-1.30 (latest) ✓
Downstream, on main fails — release-1.29 not found auto-detects release-3.3.2
Any repo, no release branches skipped skipped ✓

Assisted by @claude

@fjglira fjglira requested a review from a team as a code owner April 29, 2026 16:08
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.29%. Comparing base (31f6d0f) to head (c859e83).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1897      +/-   ##
==========================================
- Coverage   80.44%   80.29%   -0.16%     
==========================================
  Files          51       51              
  Lines        2598     2598              
==========================================
- Hits         2090     2086       -4     
- Misses        385      387       +2     
- Partials      123      125       +2     
Flag Coverage Δ
integration-tests 71.16% <ø> (-0.31%) ⬇️
unit-tests 52.30% <ø> (ø)

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fjglira fjglira force-pushed the fix-linter-midstream branch from 264041e to ef9a370 Compare April 29, 2026 16:19
Copy link
Copy Markdown
Collaborator

@FilipB FilipB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this was not visible earlier.

Edit: nevermind I do now

previous_branch=$(git branch -r | grep -E 'origin/release-[0-9]+\.[0-9]+$' |
sed 's|.*origin/||' | sort -V |
awk -v target="$current_branch" '$0 == target { print prev; exit } { prev = $0 }')
elif [[ -n "${PREVIOUS_VERSION:-}" ]]; then
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As PREVIOUS_VERSION is not used anymore anywhere in this script it probably does not make sense to pass this variable to this script in the Makefile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

awk -v target="$current_branch" '$0 == target { print prev; exit } { prev = $0 }')
elif [[ -n "${PREVIOUS_VERSION:-}" ]]; then
previous_branch="release-$(echo "${PREVIOUS_VERSION}" | cut -f1,2 -d'.')"
elif [[ -n "${PREVIOUS_BRANCH:-}" ]]; then
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PREVIOUS_BRANCH is not described anywhere, maybe a short help would be good for this script.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note about PREVIOUS_BRANCH use

@fjglira
Copy link
Copy Markdown
Contributor Author

fjglira commented Apr 30, 2026

I don't understand why this was not visible earlier.

Because the change was merged 3 weeks ago and we have been dealing with the sync error since then

…checker

When PREVIOUS_VERSION is set but the derived branch (e.g. release-1.29)
doesn't exist in the cloned repo, fall back to the latest available
release-* branch instead of failing. This supports downstream repos that
use a different naming convention (e.g. release-3.x).

Also add a PREVIOUS_BRANCH env var for callers that need to specify the
exact previous branch name explicitly.

Fixes the lint-crds failure in openshift-service-mesh/sail-operator
sync PRs where the repo uses release-3.x naming.

Signed-off-by: Francisco Herrera <fjglira@gmail.com>
@fjglira fjglira force-pushed the fix-linter-midstream branch from ef9a370 to 4aa0787 Compare April 30, 2026 08:12
@fjglira
Copy link
Copy Markdown
Contributor Author

fjglira commented Apr 30, 2026

/retest

1 similar comment
@fjglira
Copy link
Copy Markdown
Contributor Author

fjglira commented Apr 30, 2026

/retest

Comment thread tools/crd-schema-checker.sh Outdated
sed 's|.*origin/||' | sort -V | tail -1)
if [[ -z "$previous_branch" ]]; then
echo "No release branches found. Skipping."
exit 0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should exit with error? If this should catch problems, we should not silently skip the test if the branch is not found

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Adding exit error when No releases branches found

Signed-off-by: Francisco Herrera <fjglira@gmail.com>
@fjglira fjglira requested a review from FilipB April 30, 2026 09:38
@istio-testing istio-testing merged commit 5ec0a1c into istio-ecosystem:main Apr 30, 2026
17 of 18 checks passed
openshift-service-mesh-bot pushed a commit to openshift-service-mesh-bot/sail-operator that referenced this pull request May 1, 2026
* upstream/main: (41 commits)
  fix: handle repos with different release branch naming in crd-schema-checker (istio-ecosystem#1897)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1899)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1896)
  Fix test docs test failure (istio-ecosystem#1890)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1892)
  Skip processing aliases referencing pre-released versions in EOL updater (istio-ecosystem#1883)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1886)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1884)
  Fixing two problems with our hack/update-istio.sh script (istio-ecosystem#1882)
  test: Modify await_operator to dinamically get deployment name from csv when OLM is true (istio-ecosystem#1874)
  Update update-deps flow with 1.30 and remove 1.27 branch (istio-ecosystem#1875)
  tests: Skip TLS profile change test when is executed on Hosted clusters (istio-ecosystem#1873)
  Add 1.30.0-alpha.2 charts (istio-ecosystem#1854)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1872)
  Using crane instead of skopeo which is not available in the build-tools (istio-ecosystem#1870)
  Sync min tls version from `TLSConfig` to `Istio` (istio-ecosystem#1859)
  Fix serves metrics securely test (istio-ecosystem#1860)
  refactor: vendor kubernetes manifests into the repo (istio-ecosystem#1853)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1856)
  Modify "download-charts" script for alpha/beta releases (istio-ecosystem#1852)
  ...
openshift-service-mesh-bot pushed a commit to openshift-service-mesh-bot/sail-operator that referenced this pull request May 4, 2026
* upstream/main: (41 commits)
  fix: handle repos with different release branch naming in crd-schema-checker (istio-ecosystem#1897)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1899)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1896)
  Fix test docs test failure (istio-ecosystem#1890)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1892)
  Skip processing aliases referencing pre-released versions in EOL updater (istio-ecosystem#1883)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1886)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1884)
  Fixing two problems with our hack/update-istio.sh script (istio-ecosystem#1882)
  test: Modify await_operator to dinamically get deployment name from csv when OLM is true (istio-ecosystem#1874)
  Update update-deps flow with 1.30 and remove 1.27 branch (istio-ecosystem#1875)
  tests: Skip TLS profile change test when is executed on Hosted clusters (istio-ecosystem#1873)
  Add 1.30.0-alpha.2 charts (istio-ecosystem#1854)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1872)
  Using crane instead of skopeo which is not available in the build-tools (istio-ecosystem#1870)
  Sync min tls version from `TLSConfig` to `Istio` (istio-ecosystem#1859)
  Fix serves metrics securely test (istio-ecosystem#1860)
  refactor: vendor kubernetes manifests into the repo (istio-ecosystem#1853)
  Automator: Update dependencies in istio-ecosystem/sail-operator@main (istio-ecosystem#1856)
  Modify "download-charts" script for alpha/beta releases (istio-ecosystem#1852)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants