opt_selector enabled multiple labels#753
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdded a private callback Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-cli/jumpstarter_cli/common.py (1)
9-11: Selector callback behavior looks correct; consider marking unused args.Joining the
multiple=Truevalues into a single comma-separated string and returningNonewhen empty keeps the CLI semantics consistent and should fix the multi-label bug without breaking existing single-usage callers.Minor:
ctxandparamare unused here and may trigger Ruff’s unused-argument rules. You could rename them to_ctx/_paramto keep linters happy:-def _opt_selector_callback(ctx, param, value): +def _opt_selector_callback(_ctx, _param, value):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-cli/jumpstarter_cli/common.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-cli/jumpstarter_cli/common.py
🧬 Code graph analysis (1)
packages/jumpstarter-cli/jumpstarter_cli/common.py (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
callback(224-225)packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1)
callback(22-24)
⏰ 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). (8)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (1)
packages/jumpstarter-cli/jumpstarter_cli/common.py (1)
15-18:opt_selectormulti-value wiring and help text are aligned with the intended behavior.Enabling
multiple=Trueand feeding the tuple through_opt_selector_callbackto get a single comma-joined selector string matches the documented behavior (“Can be specified multiple times”) and preserves backward compatibility for existing--selector key1=value1,key2=value2invocations.No functional issues spotted here; just ensure any downstream parsing logic expects a single comma-separated string, which this change still provides.
|
tested manually, it works :D |
|
thanks @mangelajo |
Head branch was pushed to by a user without write access
this is a bug fix for closing issue https://github.com/jumpstarter-dev/jumpstarter/issues/732 for multiple label selectors
Summary by CodeRabbit
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.