feat: new arg to publish command#1914
Conversation
|
|
||
| It accepts the following parameters: | ||
|
|
||
| * `--stage` - [optional] flag to choose staging splunkbase to publish the app. |
There was a problem hiding this comment.
Are we planning to expose this for all UCC users, or is this for a specific use case?
d17438a to
4f352b6
Compare
Code Coverage
|
| Type | PR | Develop | Change | Status |
|---|---|---|---|---|
| Line Coverage | 94.28% | 94.37% | 0.08% | 🔴 Decreased |
| Branch Coverage | 90.32% | 90.50% | 0.18% | 🔴 Decreased |
kkedziak-splunk
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds a --stage flag to the publish command to allow publishing to staging Splunkbase. The approach of parameterizing the base URL is reasonable.
Issues found:
-
Naming convention:
API_BASEURLuses SCREAMING_SNAKE_CASE inside a function body, which in Python conventionally indicates a module-level constant. Since this is a local variable, it should beapi_base_url(snake_case) per PEP 8. -
Generic
Exceptionraised: Incheck_package_validation, a bareraise Exception(...)is used when validation fails. This should use a more specific exception class (e.g.RuntimeErroror a customPackageValidationError) so callers can distinguish validation failures from other errors. -
Hardcoded staging URL inside function: The staging URL
https://classic.stage.splunkbase.splunk.com/api/v1is hardcoded inside a function. Consider extracting both base URLs as module-level constants for better maintainability. -
Missing test for validation failure path: The new
elsebranch incheck_package_validationthat raisesExceptionwhenresult != "pass"has no unit test covering it. -
--stagehelp text could be clearer: The help says"Whether to release the app on staging or production splunkbase"but it's a boolean flag. Suggested:"Publish to staging Splunkbase instead of production." -
No changelog entry: Adding a new CLI flag is user-facing — does the project require a changelog entry for this?
| password: str, | ||
| ) -> None: | ||
| if use_stage: | ||
| API_BASEURL = "https://classic.stage.splunkbase.splunk.com/api/v1" |
There was a problem hiding this comment.
API_BASEURL uses SCREAMING_SNAKE_CASE but this is a local variable, not a module-level constant. Per PEP 8 this should be api_base_url (snake_case).
| ) | ||
| else: | ||
| raise Exception(response_data.get("message")) | ||
| except urllib.error.HTTPError as e: |
There was a problem hiding this comment.
Raising a bare Exception is too generic. Callers cannot distinguish a validation failure from other runtime errors. Consider using RuntimeError or a custom exception class like PackageValidationError.
| username: str, | ||
| password: str, | ||
| ) -> None: | ||
| if use_stage: |
There was a problem hiding this comment.
The staging URL https://classic.stage.splunkbase.splunk.com/api/v1 is hardcoded inside the function body. Consider extracting both base URLs as module-level constants (e.g. SPLUNKBASE_API_URL and SPLUNKBASE_STAGE_API_URL) for better maintainability.
| dest="stage", | ||
| help="Whether to release the app on staging or production splunkbase.", | ||
| action="store_true", | ||
| ) |
There was a problem hiding this comment.
Help text says "Whether to release the app on staging or production splunkbase" but this is a boolean flag (present = staging, absent = production). Clearer wording: "Publish to staging Splunkbase instead of production."
Issue number:
PR Type
What kind of change does this PR introduce?
Summary
Added new argument
--stagetoucc-gen publishcommand to enable users to use staging splunkbase to publish the package.Changes
User experience
Checklist
If an item doesn't apply to your changes, leave it unchecked.
Review
Tests
See the testing doc.
Demo/meeting:
Reviewers are encouraged to request meetings or demos if any part of the change is unclear