chore(RHINENG-24199): Allow grouped hosts to move to other groups#3592
Open
johnsonm325 wants to merge 1 commit intoRedHatInsights:masterfrom
Open
chore(RHINENG-24199): Allow grouped hosts to move to other groups#3592johnsonm325 wants to merge 1 commit intoRedHatInsights:masterfrom
johnsonm325 wants to merge 1 commit intoRedHatInsights:masterfrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR removes validation that previously blocked moving hosts between non-ungrouped groups, and updates tests to confirm that adding hosts already associated with another group now results in them being moved to the target group with corresponding events emitted. Sequence diagram for adding hosts to a group with reassignment allowedsequenceDiagram
actor Client
participant API as HBI_API
participant Repo as GroupRepository
participant DB as Database
participant Events as EventProducer
Client->>API: POST /groups/{group_id}/hosts [host_id_list]
API->>Repo: _add_hosts_to_group(group_id, host_id_list, org_id)
Repo->>DB: Validate hosts exist and belong to org_id
DB-->>Repo: Existing hosts for org_id
note over Repo,DB: Previous validation blocking reassignment is removed
Repo->>DB: Remove existing HostGroupAssoc rows for host_id_list (any group)
DB-->>Repo: Existing associations removed
Repo->>DB: Insert HostGroupAssoc rows for group_id and host_id_list
DB-->>Repo: New associations committed
Repo->>Events: Emit host_group_change events for reassigned hosts
Events-->>Repo: Events accepted
Repo-->>API: Success (updated host group associations)
API-->>Client: 200 OK (hosts now in target group)
Class diagram for host group association and repository functions after changeclassDiagram
class GroupRepository {
+validate_add_host_list_to_group_for_group_create(host_id_list, group_name, org_id)
+validate_add_host_list_to_group(host_id_list, group_id, org_id)
+_add_hosts_to_group(group_id, host_id_list, org_id)
}
class HostGroupAssoc {
+host_id: uuid
+group_id: uuid
+created_on: datetime
+updated_on: datetime
}
class Group {
+id: uuid
+name: string
+org_id: string
+ungrouped: bool
}
class InventoryException {
+title: string
+detail: string
}
GroupRepository --> HostGroupAssoc : manages associations
HostGroupAssoc --> Group : many_to_one
GroupRepository ..> InventoryException : raises on invalid host_ids or org_id
class EventProducer {
+emit_host_group_change(host_id, old_group_id, new_group_id)
}
GroupRepository ..> EventProducer : emits events on reassignment
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Since the API semantics have changed from "disallow if already grouped" to "move to the new group", consider adding a short comment near the group-association logic explaining that existing associations are intentionally overwritten so future maintainers don’t reintroduce the old validation pattern.
- In the updated
test_add_associated_host_to_different_group, you may want to assert the specific host IDs present ingroup1and absent fromgroup2rather than just lengths, to better document and lock in the exact move semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since the API semantics have changed from "disallow if already grouped" to "move to the new group", consider adding a short comment near the group-association logic explaining that existing associations are intentionally overwritten so future maintainers don’t reintroduce the old validation pattern.
- In the updated `test_add_associated_host_to_different_group`, you may want to assert the specific host IDs present in `group1` and absent from `group2` rather than just lengths, to better document and lock in the exact move semantics.
## Individual Comments
### Comment 1
<location> `tests/test_api_host_group.py:197` </location>
<code_context>
+def test_add_associated_host_to_different_group(
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that host-group associations for the old groups are actually removed, not just that counts change
Currently the test only checks the lengths of `db_get_hosts_for_group(group1_id)` and `db_get_hosts_for_group(group2_id)`. To better verify the move semantics, please assert the exact host IDs for each group and that no hosts remain associated with the old group(s). For example, check that `sorted(h.id for h in db_get_hosts_for_group(group1_id)) == sorted(host_id_list)` and that `db_get_hosts_for_group(group2_id)` returns an empty list.
</issue_to_address>
### Comment 2
<location> `tests/test_api_host_group.py:196-197` </location>
<code_context>
+ # All 3 hosts should now be in group1; group2 should have 0
+ assert len(db_get_hosts_for_group(group1_id)) == 3
+ assert len(db_get_hosts_for_group(group2_id)) == 0
+ # Update events should have been produced for the moved/added hosts
+ assert event_producer.write_event.call_count == 3
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen event assertions by validating payload contents or per-host calls
Right now the test only verifies the number of calls to `write_event`. To better validate behavior, assert which hosts and event details were emitted by inspecting `event_producer.write_event.call_args_list`—e.g., confirm each call references a `host_id_list` entry and has the expected update (not failure) event metadata. This will make regressions in event contents easier to catch.
Suggested implementation:
```python
# All 3 hosts should now be in group1; group2 should have 0
assert len(db_get_hosts_for_group(group1_id)) == 3
assert len(db_get_hosts_for_group(group2_id)) == 0
# Update events should have been produced for the moved/added hosts
calls = event_producer.write_event.call_args_list
assert len(calls) == 3
# Each emitted event should correspond to one of the affected hosts
emitted_host_ids = {call.args[0]["host"]["id"] for call in calls}
assert emitted_host_ids == set(host_id_list)
# And each event should be a successful update (not a failure)
for call in calls:
event = call.args[0]
assert event["type"] == "updated"
assert event.get("status") != "failure"
```
If the actual structure of the event passed to `write_event` differs (e.g., the host id is under a different key, or the event type field is named something else, or the event is passed as a keyword argument instead of `args[0]`), adjust:
1. The `emitted_host_ids` comprehension to navigate to the real host id field.
2. The `event["type"]` and `event.get("status")` assertions to match the actual event schema (e.g., `event["event_type"]`, `event["metadata"]["result"]`, etc.).
3. If `write_event` receives multiple positional args, pick the correct index instead of `args[0]`, or switch to `call.kwargs[...]` if the event is passed by keyword.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
johnsonm325
force-pushed
the
rhineng-24199
branch
4 times, most recently
from
aa4527e to
ca62018
Compare
Collaborator
|
/retest |
johnsonm325
force-pushed
the
rhineng-24199
branch
5 times, most recently
from
987dc39 to
b78b5cb
Compare
kruai
previously approved these changes
Feb 25, 2026
Collaborator
kruai
left a comment
kruai
left a comment
There was a problem hiding this comment.
Thanks! Looks fine, just got a question.
tests/test_api_host_group.py
Outdated
| for call in calls: | ||
| event = json.loads(call.args[0]) | ||
| assert event["type"] == "updated" | ||
| assert event.get("status") != "failure" |
Collaborator
There was a problem hiding this comment.
In what scenario does it get set to "failure"?
fstavela
requested changes
Feb 26, 2026
Contributor
fstavela
left a comment
fstavela
left a comment
There was a problem hiding this comment.
You will need to update IQE tests as well; currently, they expect the old behavior.
johnsonm325
force-pushed
the
rhineng-24199
branch
from
89f7f0e to
8362d5f
Compare
Currently, HBI API returns HTTP 400 when trying to add grouped hosts to a different group. The response has this detail: "The following subset of hosts are already associated with another group: []." This should now be allowed; the host should be removed from its old group and added to the specified one.
johnsonm325
force-pushed
the
rhineng-24199
branch
from
8362d5f to
99d4eeb
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Currently, HBI API returns HTTP 400 when trying to add grouped hosts to a different group. The response has this detail: "The following subset of hosts are already associated with another group: []."
This should now be allowed; the host should be removed from its old group and added to the specified one.
Overview
This PR is being created to address RHINENG-xxxx.
(A description of your PR's changes, along with why/context to the PR, goes here.)
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Allow hosts already associated with a group to be reassigned to a different group instead of returning a 400 error.
Bug Fixes:
Tests: