Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add script to check outdated image references #2199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbaturov
Copy link
Contributor

@rbaturov rbaturov commented Feb 3, 2025

This script ensures that developers update image references when creating a new branch.

How it works:

  • A developer creates a new branch (e.g., release-4.19) and pushes it without verifying all image references are updated (using 4.19 images).
  • He then submits a PR to the release repo to add a new CI configuration for the branch.
  • If an outdated image references are detected (e.g 4.18), the script will fail the lane until they are updated and merged.

Note: I have prepared a POC to simulate this working in #2212.
This is the failure results

depends on:
#2168

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2025
Copy link
Contributor

openshift-ci bot commented Feb 3, 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

Copy link
Contributor

openshift-ci bot commented Feb 3, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rbaturov
Once this PR has been reviewed and has the lgtm label, please assign imiller0 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rbaturov
Copy link
Contributor Author

rbaturov commented Feb 4, 2025

/test all

@@ -0,0 +1,56 @@
#!/bin/bash

# Purpose: This script verifies that no files in a release branch contain references to images from the previous release version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why you are limiting this to the previous release version? Why not check for any release that doesn't match the current release. For example, if the current release is release-4.17 fail on anything matching "release-4.*" where * isn't "17".

Copy link
Contributor Author

@rbaturov rbaturov Feb 4, 2025

Choose a reason for hiding this comment

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

I was thinking of implementing it by using a regex pattern like 4.* to match versions that are not equal to the current release. However, I’m concerned that this approach might lead to false positives since the 4.* pattern is too broad. There might be a smarter way to handle this—I’m still thinking it through, and this is just a draft. 😊

However, since I am aligning the master branch in all these references to the new release branch (4.19), there should theoretically be no scenario in the future where we skip two or more versions. This is ensured by the script, which would fail the CI and prevent such a situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

In what cases would the "release-4.*" pattern be too broad? I still think checking for any release that doesn't match the current one is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bartwensley we are grep-ing "4.* " and not "release-4.* ", as most of the image references (if not all) don't have the release prefix. Look here for example.
"4.*" is a pattern which will potentially fail us on false positives.

@rbaturov rbaturov force-pushed the verify-image-ref-consistent branch from a87f554 to 9320f96 Compare February 4, 2025 13:47
@rbaturov
Copy link
Contributor Author

rbaturov commented Feb 4, 2025

/test all

@rbaturov rbaturov mentioned this pull request Feb 4, 2025
@rbaturov
Copy link
Contributor Author

rbaturov commented Feb 4, 2025

/retest

@rbaturov rbaturov mentioned this pull request Feb 4, 2025
@rbaturov rbaturov marked this pull request as ready for review February 5, 2025 10:27
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2025
@openshift-ci openshift-ci bot requested review from swatisehgal and Tal-or February 5, 2025 10:27

x="${BASH_REMATCH[1]}"
y="${BASH_REMATCH[2]}"
previous_version="$x.$((y - 1))" # Example: 4.16 for branch release-4.17

Choose a reason for hiding this comment

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

What if the branch was significantly outdated and contained references older than the previous release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't be possible, look at my comment to Bart. Since we aligned the master branch to reference 4.19 images and assuming having this script this would be impossible

hack/verify-images-updated.sh Outdated Show resolved Hide resolved
- Detects release branch naming (e.g., `release-x.y`) and identifies the previous version.
- Scans specified files for references to the previous version.
- Ensures files are updated for the current release branch, aiding consistency.

This script helps developers creating a new branch to remember updating image references.

Signed-off-by: Ronny Baturov <[email protected]>
@rbaturov rbaturov force-pushed the verify-image-ref-consistent branch from 9320f96 to 9429de9 Compare February 5, 2025 14:25
@rbaturov
Copy link
Contributor Author

rbaturov commented Feb 5, 2025

/retest

Copy link
Contributor

openshift-ci bot commented Feb 5, 2025

@rbaturov: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ci-tests 9429de9 link true /test e2e-aws-ci-tests
ci/prow/e2e-aws-ran-profile 9429de9 link false /test e2e-aws-ran-profile

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants