Sanchaga/pr 58 reviewbottest#68
Conversation
…ruct-fix PSAAS-24764: Move code to 'package' directory
…wska/PSAAS-24765-get-action
…-soar-connectors/http into msankowska/PSAAS-24765-get-action
PSAAS-24767- tested actions + POST implementation
PSAAS-24768 put request
PSAAS-25369 patch_data, get_headers, delete_data, get_options requests
PSAAS-25413 - put and get file
PSAAS-25399 code refactoring
| if not validators.url(full_url): | ||
| raise ActionFailure(f"Invalid URL constructed: {full_url}") | ||
|
|
||
| with vault_attachment.open() as f: |
There was a problem hiding this comment.
Issue: response = requests.post(uri=full_url, auth=auth_object, ...) — requests.post() has no 'uri' parameter; the correct keyword is 'url'.
Suggested fix: Change 'uri=full_url' to 'url=full_url'.
| method="DELETE", | ||
| location=params.location, | ||
| headers=params.headers, | ||
| verify=params.verify_certificate, |
There was a problem hiding this comment.
Issue: DeleteDataParams declares 'body: str' and PatchDataParams declares 'body: str', but both action handlers call make_request(..., body=None) instead of body=params.body.
Suggested fix: Change body=None to body=params.body in action_delete.py and action_patch.py make_request calls.
| method=asset.test_http_method, | ||
| location=asset.test_path if asset.test_path else "", | ||
| output=EmptyOutput, | ||
| verify=False, |
There was a problem hiding this comment.
Issue: make_request(..., verify=False, ...) is hardcoded in the test_connectivity handler.
Suggested fix: Add a verify_certificate asset config field (defaulting to True) and pass asset.verify_certificate here, consistent with how action-level verify_certificate works.
| product_vendor="Generic", | ||
| product_name="HTTP", | ||
| publisher="Splunk", | ||
| appid="290b7499-0374-4930-9cdc-5e9b05d65827", |
There was a problem hiding this comment.
Issue: Old http.json had 'fips_compliant: true'.
Suggested fix: Verify whether the new SDK and dependencies are FIPS-compatible.
|
|
||
| jobs: | ||
| call-push: | ||
| uses: splunk-soar-connectors/.github/.github/workflows/push.yml@tapishj/PAPP-36785-update-ci-for-sdkfied-apps |
There was a problem hiding this comment.
Issue: uses: splunk-soar-connectors/.github/.github/workflows/push.yml@tapishj/PAPP-36785-update-ci-for-sdkfied-apps — this pins CI to a personal branch that can be force-pushed or deleted at any time.
Suggested fix: Pin to a stable tag or the main branch of the shared workflow repo once the referenced branch is merged.
|
Issue: CI shows: pre-commit: failure; aggregate-sanity-results: failure; sanity-test failures on all platforms (previous, cloud, cloud_next, next_ol8, next_amzn2023, next_ol9); integration-test: failure. Suggested fix: Resolve pre-commit hook failures locally, fix the CI workflow ref (see ci-workflow-feature-branch-ref finding), and ensure the new package structure passes sanity tests before re-review. |
| primary=True, | ||
| cef_types=["file path"], | ||
| ) | ||
| verify_certificate: bool = Param( |
There was a problem hiding this comment.
Issue: verify_certificate: bool = Param(description='Verify certificates (if using HTTPS)', default=False) — same pattern in put_file.py line 42.
Suggested fix: Change default=False to default=True for verify_certificate in both get_file and put_file Params classes.
| @@ -0,0 +1,140 @@ | |||
| [project] | |||
| name = "http" | |||
| version = "4.0.0" | |||
There was a problem hiding this comment.
Issue: package/pyproject.toml sets version = '4.0.0' while the base http.json (still present as a base file) shows app_version = '3.7.9'.
Suggested fix: Align the version across pyproject.toml and any generated/checked-in app JSON.
|
|
||
| body = UnicodeDammit(body).unicode_markup.encode("utf-8") if isinstance(body, str) else body | ||
|
|
||
| with temp_cert_files(asset.public_cert, asset.private_key) as cert_param: |
There was a problem hiding this comment.
Issue: This changes the client certificate auth path, but I don't see test coverage for it.
Suggested fix: Add tests for valid cert/key input, the no-cert path, invalid certificate/key input, and that the request sends the expected cert value.
3 participants
Features
List any features you're adding to this app (e.g. new actions, parameters, auth methods, etc.)
Bug Fixes
Describe any bugs you've fixed in this app, and how they were fixed
Manual Documentation
Have you made any changes that should be documented in manual_readme_content.md?
The following changes require documentation in
manual_readme_content.md:Other information
Please refer to our Contribution Guide for any questions on submitting a pull request.
Thanks for contributing!