-
Notifications
You must be signed in to change notification settings - Fork 1
fix Ci #162
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request updates the CI/CD workflow to explicitly lock Poetry dependencies before installation via two new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
python/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.github/workflows/python-package.yml(2 hunks)python/pyproject.toml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (macos-latest, 3.11)
- GitHub Check: build (macos-latest, 3.10)
- GitHub Check: build (windows-latest, 3.14)
🔇 Additional comments (2)
python/pyproject.toml (2)
32-32: psutil is actually being used—the review comment is unfounded.The dependency isn't mysterious or dead code. It's explicitly imported and used in
python/tests/test_backup_multi_bucket.pyto get process information. It's also pulled in as a transitive dependency by several packages (dmypy, optional extras for other dependencies). No action needed—this is solid engineering.Likely an incorrect or invalid review comment.
16-16: Let me dig deeper into the repository context to understand if this constraint change was intentional.Now let me check the repository context to understand whether this change was intentional.
Based on my verification, let me generate the final response:
The constraint relaxation is accurate, but the terminology is misleading.
Changed from
> 3.20.0to>=3.20.0—this allows filelock 3.20.0, which was previously excluded. This is a constraint relaxation, not "standardizing" as the AI summary claims.The good news: filelock 3.20.0 is actively packaged and available across distributions (Arch Linux, Debian, MSYS2), and there are no known issues with that version. Allowing it expands compatibility without risk.
The open question: Was this change intentional, or did someone flip the operator without thinking? Without commit context, it's hard to say. If you're relaxing constraints deliberately for better dependency compatibility, this is fine. If it was accidental, you should clarify the constraint logic.
| cd python | ||
| poetry lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Lock generation on every build is sound, but verify it aligns with your lock file strategy.
You're running poetry lock before install to update the lock file to match pyproject.toml without installing dependencies. This is legitimate practice for catching broken dependencies early. However, the pattern regenerates locks on every CI run across multiple Python/OS combinations, which could lead to:
- Lock file diffs appearing in CI artifacts if not committed properly
- Platform-specific lock differences if lock file isn't standardized
Suggestion: Commit the poetry.lock file to your version control system to ensure all developers and CI environments use identical dependency versions. If already doing that, consider whether you need poetry lock each run or only when dependencies change.
Also applies to: 58-59
🤖 Prompt for AI Agents
.github/workflows/python-package.yml lines 35-36 (and similarly 58-59): CI runs
`poetry lock` on every build which can cause uncommitted lock diffs and
platform-specific lock divergence; ensure you commit poetry.lock to the repo and
remove the unconditional `poetry lock` step from CI (or make it conditional/run
only when pyproject.toml changes) so CI uses the committed lockfile consistently
across platforms.
python/pyproject.toml
Outdated
| [tool.poetry] | ||
| name = "bucketbase" | ||
| version = "1.5.0" # do not edit manually. kept in sync with `tool.commitizen` config via automation | ||
| version = "1.5.1" # do not edit manually. kept in sync with `tool.commitizen` config via automation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version sync is broken — tool.poetry and tool.commitizen are out of sync.
Line 3 says the version is kept in sync with tool.commitizen via automation, but you've bumped poetry to 1.5.1 while commitizen is still 1.2.3. If the sync is real, either the automation failed or wasn't run. If it's aspirational, that comment is misleading.
Either:
- Update commitizen to 1.5.1 to actually keep them in sync, or
- Remove the sync comment if automation isn't real.
-version = "1.2.3" # do not edit manually. kept in sync with `tool.poetry` config via automation
+version = "1.5.1" # do not edit manually. kept in sync with `tool.poetry` config via automationAlso applies to: 80-80
🤖 Prompt for AI Agents
In python/pyproject.toml around lines 3 and 80, the file claims the version is
"kept in sync with `tool.commitizen` via automation" but the versions differ
(poetry bumped to 1.5.1 while commitizen remains 1.2.3); either update the
commitizen version to match 1.5.1 (and run the automation if required) or
remove/adjust the misleading sync comment so it accurately reflects reality;
make the change consistently in both occurrences (line 3 and line 80).
Title
<type>:<short summary>Ticket / Task
<URL>What & Why
"Builds X so that Y can Z" – two sentences max.
Scope
Checklist (self-review)
See Section 5.
Risk / Impact
Screenshots / Logs / Benchmarks
(only if relevant)
Author Self-Review Checklist
pytest -q/mvn testclean locally.git revertwithout dependency hell.python/scripts/run_mypy.batlocally with no critical errorsResources: