-
Notifications
You must be signed in to change notification settings - Fork 127
Convert Lightspeed inventory sync from CLI to UI #20028
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
Convert Lightspeed inventory sync from CLI to UI #20028
Conversation
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:
- There’s a typo in the decorator: you’ve used
@pytest.mark_pit_client
instead of@pytest.mark.pit_client
, so that marker won’t actually be applied. - The UI test never asserts any visible confirmation after calling
session.cloudinventory.sync_inventory_status()
; consider adding a check for a toast or status indicator to ensure the sync was initiated in the UI before polling the API. - The timestamp +
wait_for
polling logic is duplicated here and in other tests—extract that into a shared helper to avoid repetition and make the test easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a typo in the decorator: you’ve used `@pytest.mark_pit_client` instead of `@pytest.mark.pit_client`, so that marker won’t actually be applied.
- The UI test never asserts any visible confirmation after calling `session.cloudinventory.sync_inventory_status()`; consider adding a check for a toast or status indicator to ensure the sync was initiated in the UI before polling the API.
- The timestamp + `wait_for` polling logic is duplicated here and in other tests—extract that into a shared helper to avoid repetition and make the test easier to maintain.
## Individual Comments
### Comment 1
<location> `tests/foreman/cli/test_rhcloud_inventory.py:548` </location>
<code_context>
def generate_report(rhcloud_manifest_org, module_target_sat, disconnected=False):
"""Download generated report job via cli"""
org = rhcloud_manifest_org
params = {'organization-id': org.id}
if disconnected:
params['no-upload'] = True
result = module_target_sat.cli.Insights.inventory_generate_report(params)
success_msg = "Report generation started successfully"
timestamp = datetime.now(UTC).strftime('%Y-%m-%d %H:%M')
assert success_msg in result
# Check task details
generate_job_name = 'ForemanInventoryUpload::Async::GenerateReportJob'
wait_for(
lambda: module_target_sat.api.ForemanTask()
.search(query={'search': f'{generate_job_name} and started_at >= "{timestamp}"'})[0]
.result
== 'success',
timeout=400,
delay=15,
silent_failure=True,
handle_exception=True,
)
task_output = module_target_sat.api.ForemanTask().search(
query={'search': f'{generate_job_name} and started_at >= "{timestamp}"'}
)
assert task_output[0].result == "success"
report_log = module_target_sat.api.Organization(id=org.id).rh_cloud_fetch_last_report_log()
expected = f'Check the Uploading tab for report uploading status'
assert expected in report_log['output']
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace f-string with no interpolated values with string ([`remove-redundant-fstring`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-fstring/))
```suggestion
expected = 'Check the Uploading tab for report uploading status'
```
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ac37c1a
to
9da6946
Compare
|
PRT Result
|
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.
LGTM
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.
Looks good to me, just a few comments on typos :)
@pytest.mark.pit_server | ||
@pytest.mark.pit_client | ||
def test_sync_inventory_status(rhcloud_manifest_org, rhcloud_registered_hosts, module_target_sat): | ||
"""Test syncing Lighstpeed inventory status |
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.
Fix typo here
:id: 1fe47111-8831-4168-b79e-ca7c1f6aa343 | ||
:steps: | ||
1. Register 2 hosts to Satellite to an org with a manifest importedand set up Lightspeed. |
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.
and here
The CLI test for syncing Lightpseed inventory status is failing due to its reliance on the `foreman-rake rh_cloud_inventory:sync` rake task. This command syncs the inventory for all organization on a Satellite and does not respect options passed in to restrict the scope of the command to a single org. As a result, in CI, when the test searches for an inventory sync task on which to make assertions, it often erroneously finds a sync task for an organization other than the one being tested. Additionally, syncing the inventory via the `foreman-rake` CLI is not a documented approach, and the `foreman-rake` CLI is typically not a supported customer use case. This PR converts the CLI inventory sync test to a UI test, which uses a supported workflow and restricts the sync to the selected org by default.
9da6946
to
02884d0
Compare
* Convert Lightspeed inventory sync from CLI to UI The CLI test for syncing Lightpseed inventory status is failing due to its reliance on the `foreman-rake rh_cloud_inventory:sync` rake task. This command syncs the inventory for all organization on a Satellite and does not respect options passed in to restrict the scope of the command to a single org. As a result, in CI, when the test searches for an inventory sync task on which to make assertions, it often erroneously finds a sync task for an organization other than the one being tested. Additionally, syncing the inventory via the `foreman-rake` CLI is not a documented approach, and the `foreman-rake` CLI is typically not a supported customer use case. This PR converts the CLI inventory sync test to a UI test, which uses a supported workflow and restricts the sync to the selected org by default. * Address reviewer feedback (cherry picked from commit 3ee1bef)
Problem Statement
The CLI test for syncing Lightpseed inventory status is failing due to its reliance on the
foreman-rake rh_cloud_inventory:sync
rake task. This command syncs the inventory for all organization on a Satellite and does not respect options passed in to restrict the scope of the command to a single org. As a result, in CI, when the test searches for an inventory sync task on which to make assertions, it often erroneously finds a sync task for an organization other than the one being tested.Additionally, syncing the inventory via the
foreman-rake
CLI is not a documented approach, and theforeman-rake
CLI is typically not a supported customer use case.Solution
Convert the CLI inventory sync test to a UI test, which uses a supported workflow and restricts the sync to the selected org by default.