Repository navigation
NO-ISSUE: Add migration file list hash to prevent numbering collisions - #747
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (6)
WalkthroughAdds a SHA-256 hash sentinel over sorted migration filenames. A new Python Click subcommand computes and writes the hash to ChangesMigration collision guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 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 |
jhernand
changed the title
|
@jhernand: This pull request explicitly references no jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/database/migrations/migrations_suite_test.go`:
- Around line 80-87: The regex pattern `[0-9a-f]{64}` is too generic and will
match any 64-character hex string in the file, potentially replacing unintended
hashes if the README includes other examples. Modify the `hashPattern` variable
to include surrounding context that uniquely identifies the migration hash, such
as code fence delimiters (triple backticks) or other structural markers from the
README format. This ensures that `ReplaceAllString` only targets the actual
migration hash location and not any other 64-hex strings that might exist
elsewhere in the file.
- Around line 74-89: The test currently extracts the storedHash from README.md
and then unconditionally replaces it with computedHash without verifying they
match first. After finding storedHash using the hashPattern regex and before
calling hashPattern.ReplaceAllString to replace the hash in readmeText, add an
Expect assertion that verifies storedHash equals computedHash. If the hashes
don't match, the test must fail with a clear error message indicating that the
migration file list has changed and the README.md hash needs to be updated and
committed. This ensures developers cannot merge without committing the updated
hash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 03cf95f0-c85d-416f-87e4-2200dc9d449c
📒 Files selected for processing (2)
internal/database/migrations/README.mdinternal/database/migrations/migrations_suite_test.go
|
As long as we're using the merge queue that runs the E2E tests before merging, this error will be checked there anyhow. But, I do understand the advantage of having it here. I agree with this comment: #747 (comment). You want to fail the test to ensure that the change to the README is not forgotten. Alternatively, just have it fail and print the expected value, forcing the user to update the value in the README. Yes, it's extra work for the user, but I'm not sure that tests with side effects are the right thing to do. |
|
/hold |
f839b01 to
c5fa796
Compare
The new version of the patch should address these concerns: the unit tests only verifies that the hash is correct, and suggest the user to either fix the file manually or else use the |
|
/unhold |
c5fa796 to
21abe1d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/database/database_migrations_test.go`:
- Around line 119-120: The hash loop in the database migrations test is ignoring
the error return from fmt.Fprintf, which violates the Go error-handling rule.
Update the loop that writes names into computedHashSource to capture the
returned error, assert it in the test, and fail immediately if the write does
not succeed so the hashing setup is validated reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 46392613-41db-4667-bb16-843a52eabd1d
📒 Files selected for processing (6)
dev.pydev/__init__.pydev/update.pyinternal/database/README.mdinternal/database/database_migrations_test.gointernal/database/migrations.sha256
Add a `migrations.sha256` file in `internal/database/` containing the SHA-256 hash of the sorted list of `.up.sql` migration filenames. Its purpose is to cause a git merge conflict when two pull requests independently introduce migrations with the same number. A unit test in `database_migrations_test.go` verifies that the stored hash matches the current migration file list. When the hash is outdated the test failure message points to the `uv run dev.py update hashes` command, which recomputes and writes the hash automatically. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
21abe1d to
6a60b16
Compare
|
💀 CI Triage: Root cause: Helm upgrade fails during the boot/refresh phase because of a strategic merge patch conflict on the osac-operator deployment's OSAC_AAP_TOKEN environment variable. Explanation: Recently merged PR #328 in osac-installer configured the operator to load its AAP token from a secret via Evidence: Suggestion: To fix this, either: (1) update the osac-operator Helm chart deployment template to explicitly set Prow job | Build For deeper investigation, use the |
|
/retest |
4 participants
Add a
migrations.sha256file ininternal/database/containing the SHA-256 hash of the sortedlist of
.up.sqlmigration filenames. Its purpose is to cause a git merge conflict when two pullrequests independently introduce migrations with the same number.
A unit test in
database_migrations_test.goverifies that the stored hash matches the currentmigration file list. When the hash is outdated the test failure message points to the
uv run dev.py update hashescommand, which recomputes and writes the hash automatically.Summary by CodeRabbit
Summary
New Features
hasheshelper).Bug Fixes
Documentation