To support session-name option to the subset command
#1001
Conversation
According to the documentation, it's supposed to be supported, but the subset isn't actually supported
WalkthroughA new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LaunchableAPI
User->>CLI: launch subset --session-name NAME --build BUILD
CLI->>CLI: Validate both --session-name and --build are provided
CLI->>LaunchableAPI: GET /builds/{build_name}/test_session_names/{session_name}
LaunchableAPI-->>CLI: Responds with session ID
CLI->>CLI: Construct session identifier using session ID
CLI->>CLI: Proceed with subset logic
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (13)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
launchable/commands/subset.py (1)
275-284: Fix grammatical error in the error message.The error message contains a small grammatical error.
- '--build option is required when you uses a --session-name option ') + '--build option is required when you use a --session-name option')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
launchable/commands/subset.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: build (windows-latest, 3.6)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (windows-latest, 3.8)
- GitHub Check: build (windows-latest, 3.7)
- GitHub Check: build (ubuntu-22.04, 3.10)
- GitHub Check: build (ubuntu-22.04, 3.9)
- GitHub Check: build (ubuntu-22.04, 3.8)
- GitHub Check: build (ubuntu-22.04, 3.7)
🔇 Additional comments (4)
launchable/commands/subset.py (4)
162-169: Correctly adds support for the--session-nameoption.This change properly implements the
--session-nameoption to align with documented behavior, addressing the issue described in the PR objectives.
221-221: Properly integrates thesession_nameparameter.The function signature is correctly updated with the optional parameter that has appropriate type annotation and default value.
275-296: Logic for session name resolution looks good.The implementation correctly:
- Requires
--buildwhen--session-nameis specified- Makes an API request to resolve the session name to an ID
- Constructs the session identifier properly
- Falls back to existing session handling logic when
--session-nameis not providedThe error handling is also appropriate for handling configuration issues.
297-304: Error handling is properly implemented.This error handling ensures that when
--session-nameis used incorrectly (without the required--buildoption), the CLI displays a clear error message and exits with status code 1, preventing further processing as specified in the PR objectives.
| test_suite=test_suite, | ||
| ) | ||
| except click.UsageError as e: | ||
| click.echo( |
There was a problem hiding this comment.
Also, I'd like to capture this exception in the telemetry.
|
According to the documentation, it's supposed to be supported, but the subset isn't actually supported
ref: https://www.launchableinc.com/docs/resources/cli-reference/#3500790-subset
Even in the record tests command with the session-name option, the build option is required.
So for case the session-name option is set and the build option isn't set, the CLI treated it as a miss integration, and made it exit with status code 1 to prevent the process from continuing.
Checked Locally
Summary by CodeRabbit
--session-nameoption to the subset command to specify a test session by name.--session-nameoption with clearer user feedback on incorrect usage.