-
Notifications
You must be signed in to change notification settings - Fork 90
[6.18.z] Add bulk applicable and installable errata endpoints #1389
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
Open
Satellite-QE
wants to merge
1
commit into
6.18.z
Choose a base branch
from
cherry-pick-6.18.z-972a39a2a51ab8b11978eea10fd8aac63b8e1dfe
base: 6.18.z
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
[6.18.z] Add bulk applicable and installable errata endpoints #1389
Satellite-QE
wants to merge
1
commit into
6.18.z
from
cherry-pick-6.18.z-972a39a2a51ab8b11978eea10fd8aac63b8e1dfe
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(cherry picked from commit 972a39a)
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider refactoring bulk_applicable_errata and bulk_installable_errata to use a shared helper for bulk POST endpoints and reduce code duplication.
- Add tests to verify the actual endpoint URL resolution for the new errata methods, not just the HTTP verb, to ensure the paths are correct.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider refactoring bulk_applicable_errata and bulk_installable_errata to use a shared helper for bulk POST endpoints and reduce code duplication.
- Add tests to verify the actual endpoint URL resolution for the new errata methods, not just the HTTP verb, to ensure the paths are correct.
## Individual Comments
### Comment 1
<location> `tests/test_entities.py:2181` </location>
<code_context>
(entities.Host(**generic).bulk_destroy, 'put'),
(entities.Host(**generic).bulk_traces, 'post'),
(entities.Host(**generic).bulk_resolve_traces, 'put'),
+ (entities.Host(**generic).bulk_applicable_errata, 'post'),
+ (entities.Host(**generic).bulk_installable_errata, 'post'),
(entities.HostGroup(**generic).add_puppetclass, 'post'),
</code_context>
<issue_to_address>
**suggestion (testing):** Missing edge case tests for bulk_applicable_errata endpoint.
Please add tests for empty host lists, invalid host IDs, server error responses (4XX/5XX), and 202 status code handling to improve coverage of bulk_applicable_errata.
Suggested implementation:
```python
(entities.Host(**generic).bulk_applicable_errata, 'post'),
```
```python
def test_bulk_applicable_errata_empty_list(client):
"""Test bulk_applicable_errata with empty host list."""
response = client.post('/api/hosts/bulk_applicable_errata', json={"host_ids": []})
assert response.status_code == 400 or response.status_code == 422 # Expecting client error for empty input
def test_bulk_applicable_errata_invalid_host_ids(client):
"""Test bulk_applicable_errata with invalid host IDs."""
response = client.post('/api/hosts/bulk_applicable_errata', json={"host_ids": ["not-a-real-id", 999999]})
assert response.status_code == 404 or response.status_code == 400 # Expecting not found or bad request
def test_bulk_applicable_errata_server_error(monkeypatch, client):
"""Test bulk_applicable_errata handling server error responses."""
def mock_bulk_applicable_errata(*args, **kwargs):
from flask import abort
abort(500)
monkeypatch.setattr(entities.Host, "bulk_applicable_errata", mock_bulk_applicable_errata)
response = client.post('/api/hosts/bulk_applicable_errata', json={"host_ids": [1, 2, 3]})
assert response.status_code == 500
def test_bulk_applicable_errata_202_status(monkeypatch, client):
"""Test bulk_applicable_errata returns 202 status code."""
from flask import jsonify, make_response
def mock_bulk_applicable_errata(*args, **kwargs):
return make_response(jsonify({"message": "Accepted"}), 202)
monkeypatch.setattr(entities.Host, "bulk_applicable_errata", mock_bulk_applicable_errata)
response = client.post('/api/hosts/bulk_applicable_errata', json={"host_ids": [1, 2, 3]})
assert response.status_code == 202
assert response.get_json()["message"] == "Accepted"
```
</issue_to_address>
### Comment 2
<location> `tests/test_entities.py:2182` </location>
<code_context>
(entities.Host(**generic).bulk_traces, 'post'),
(entities.Host(**generic).bulk_resolve_traces, 'put'),
+ (entities.Host(**generic).bulk_applicable_errata, 'post'),
+ (entities.Host(**generic).bulk_installable_errata, 'post'),
(entities.HostGroup(**generic).add_puppetclass, 'post'),
(entities.HostGroup(**generic).assign_ansible_roles, 'post'),
</code_context>
<issue_to_address>
**suggestion (testing):** Missing edge case tests for bulk_installable_errata endpoint.
Please add tests for empty or invalid host lists, error responses, and both synchronous/asynchronous 202 responses to ensure comprehensive coverage.
Suggested implementation:
```python
(entities.Host(**generic).bulk_installable_errata, 'post'),
```
```python
def test_bulk_installable_errata_empty_host_list(client):
"""Test bulk_installable_errata with empty host list."""
response = client.post('/api/hosts/bulk_installable_errata', json={"hosts": []})
assert response.status_code == 400 or response.status_code == 422
assert "host list" in response.json.get("error", "")
def test_bulk_installable_errata_invalid_host_list(client):
"""Test bulk_installable_errata with invalid host list."""
# Host list is not a list
response = client.post('/api/hosts/bulk_installable_errata', json={"hosts": "notalist"})
assert response.status_code == 400 or response.status_code == 422
assert "invalid" in response.json.get("error", "")
# Host list missing required fields
response = client.post('/api/hosts/bulk_installable_errata', json={"hosts": [{}]})
assert response.status_code == 400 or response.status_code == 422
assert "missing" in response.json.get("error", "")
def test_bulk_installable_errata_error_response(client, mocker):
"""Test bulk_installable_errata error responses."""
# Simulate internal server error
mocker.patch("entities.Host.bulk_installable_errata", side_effect=Exception("Internal error"))
response = client.post('/api/hosts/bulk_installable_errata', json={"hosts": [{"id": 1}]})
assert response.status_code == 500
assert "Internal error" in response.json.get("error", "")
def test_bulk_installable_errata_sync_response(client, mocker):
"""Test bulk_installable_errata synchronous 200/201 response."""
mocker.patch("entities.Host.bulk_installable_errata", return_value={"result": "ok"})
response = client.post('/api/hosts/bulk_installable_errata', json={"hosts": [{"id": 1}]})
assert response.status_code in (200, 201)
assert response.json.get("result") == "ok"
def test_bulk_installable_errata_async_response(client, mocker):
"""Test bulk_installable_errata asynchronous 202 response."""
mocker.patch("entities.Host.bulk_installable_errata", return_value=None)
response = client.post('/api/hosts/bulk_installable_errata', json={"hosts": [{"id": 1}]})
assert response.status_code == 202
assert "accepted" in response.json.get("message", "").lower()
```
- Ensure that the test client fixture (`client`) and mocking library (`mocker`) are available in your test setup.
- Adjust endpoint paths and error message keys if your API differs.
- If your API uses a different error response format, update the assertions accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
This pull request has not been updated in the past 45 days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
6.18.z
Auto_Cherry_Picked
GHA has automatically cherrypicked this PR
No-CherryPick
PR doesnt need CherryPick to previous branches
Stale
Stale issue or Pull Request
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherrypick of PR: #1375
Description of changes
Adding two errata related endpoints.
Upstream API documentation, plugin, or feature links
SAT/apidoc/v2/hosts_bulk_actions/applicable_errata.en.html
SAT/apidoc/v2/hosts_bulk_actions/installable_errata.en.html
Functional demonstration
Robottelo PR: SatelliteQE/robottelo#20193