fix: Check status by SHA in wait check#2459
Conversation
Added logic to find the correct status by SHA when checking for a succeeded condition. Previously, it always used the last status, which could lead to incorrect assertions if multiple statuses existed with different SHAs. This change ensures the check is performed against the status corresponding to the provided SHA.
Summary of ChangesHello @chmouel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential issue in status checking logic where the system might incorrectly assert against the latest status instead of the one corresponding to a specific commit SHA. By introducing logic to explicitly search for and utilize the status associated with a given SHA, the changes enhance the reliability and accuracy of status condition evaluations, particularly in scenarios involving multiple status updates for different commits. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where the wrong status was being checked by always using the last one. The changes ensure that when a SHA is provided, the corresponding status is found and used. My review focuses on improving the robustness of the test code. The suggested improvement in test/pkg/wait/check.go to fail fast if the status for a given SHA is not found will make test failures clearer. The changes in test/pkg/wait/wait.go look correct and ensure the polling logic waits for the specific SHA to appear.
There was a problem hiding this comment.
Pull request overview
Improves the E2E wait/assert helpers to select the repository run status that matches a provided commit SHA, avoiding incorrect assertions when repo.Status contains entries for multiple SHAs.
Changes:
- Update
Succeeded()check to searchrepo.Statusfor the most recent entry matchingsopt.SHA. - Update
UntilRepositoryUpdated()to wait until a status entry forTargetSHAappears (in addition to the existing min-status wait).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
test/pkg/wait/wait.go |
Adds TargetSHA-aware gating in the repository-status wait loop. |
test/pkg/wait/check.go |
Selects the status entry to assert on by matching SHA instead of always using the last status. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Improved the logic for checking repository status by SHA. Previously, it only checked if any status entry contained the target SHA. Now, it verifies that a specific number of status entries match the target SHA, providing a more robust check. Additionally, it was enhanced to report all available SHAs if a matching SHA is not found, aiding in debugging. This change ensures that tests accurately reflect the expected status updates for a given commit. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
09e6919 to
ceb3911
Compare
Added retry logic to the repository URL renaming process during teardown. This addresses potential conflicts arising from optimistic locking when the repository's resource version changes between listing and updating, ensuring the teardown completes reliably.
|
let's give this a try |
📝 Description of the Change
Fix SHA matching in test wait/check utilities to avoid incorrect assertions when multiple repository statuses exist.
test/pkg/wait/check.go:Succeeded(), find the matching status entry by iterating backwards instead of blindly using the last status. This prevents false failures when multiple statuses exist with different SHAs.test/pkg/wait/wait.go:UntilRepositoryUpdated(), whenTargetSHAis set, count only the status entries matching that specific SHA rather than just checking total status count. This ensures we wait for the correct SHA's status to appear.👨🏻 Linked Jira
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.