refactor(RHINENG-22723): remove canonical_facts from MQ #3294
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
Contributor
Reviewer's GuideRefactors host serialization, MQ event construction, and ownership logic to stop using the canonical_facts dict, instead reading canonical identifiers directly from Host model fields and updating tests and helpers to operate in a Flask app context and validate the new behavior. Sequence diagram for host MQ add/update event using Host field-based identifierssequenceDiagram
participant ApiHost
participant Host
participant Serialization
participant HostMQ
participant EventProducer
participant Cache
ApiHost->>Host: apply update (set insights_id, subscription_manager_id, fqdn)
ApiHost->>Serialization: serialize_host(host, staleness_timestamps, staleness)
Serialization->>Host: serialize_canonical_facts(host)
Serialization-->>ApiHost: serialized_host
ApiHost->>HostMQ: write_add_update_event_message(event_producer, result)
HostMQ->>Serialization: serialize_host(result.row, result.staleness_timestamps, staleness)
Serialization->>Host: read CANONICAL_FACTS_FIELDS from attributes
Serialization-->>HostMQ: output_host (canonical facts from Host fields)
HostMQ->>Host: read insights_id
HostMQ->>EventProducer: write_event(event, host.id, headers with insights_id)
alt cache_cleanup_needed
HostMQ->>Host: read insights_id and system_profile_facts.owner_id
HostMQ->>Cache: delete_cached_system_keys(insights_id, org_id, owner_id)
end
Class diagram for Host model and related serialization/MQ changesclassDiagram
class Config {
+ID_FACTS
+ID_FACTS_USE_SUBMAN_ID
+CANONICAL_FACTS_FIELDS
}
class Host {
+UUID id
+String account
+String org_id
+String display_name
+String ansible_host
+JSONB canonical_facts
+UUID insights_id
+UUID subscription_manager_id
+UUID satellite_id
+String fqdn
+String bios_uuid
+JSONB ip_addresses
+JSONB mac_addresses
+String provider_id
+String provider_type
+UUID openshift_cluster_id
+JSONB system_profile_facts
+String reporter
+dict per_reporter_staleness
+__init__(canonical_facts=None, display_name=None, ansible_host=None, account=None, org_id=None, facts=None, reporter=None, tags=None, system_profile_facts=None, groups=None, insights_id=None, subscription_manager_id=None, satellite_id=None, fqdn=None, bios_uuid=None, ip_addresses=None, mac_addresses=None, provider_id=None, provider_type=None, openshift_cluster_id=None)
+update(input_host, update_system_profile=False) void
+save() Host
+update_canonical_facts(canonical_facts) void
+update_canonical_facts_columns(canonical_facts) void
+__repr__() String
}
class Serialization {
+serialize_host(host, staleness_timestamps, rbac_filtered=False, staleness=None, fields=None, host_type=None) dict
+serialize_canonical_facts(host) dict
+deserialize_canonical_facts(raw_data, all=False) dict
+remove_null_canonical_facts(serialized_host) void
+_deserialize_canonical_facts(data) dict
+_deserialize_all_canonical_facts(data) dict
}
class Events {
+host_delete_event(event_type, host, initiated_by_frontend=False, platform_metadata=None) dict
}
class HostMQ {
+_set_owner(host, identity) Host
+sync_event_message(message, session, event_producer) None
+write_delete_event_message(event_producer, result) None
+write_add_update_event_message(event_producer, result) None
}
class ApiHost {
+_emit_patch_event(serialized_host, host) None
+patch_host_by_id(host_id_list, body, rbac_filter=None) dict
+update_facts_by_namespace(operation, host_id_list, namespace, fact_dict, rbac_filter=None) dict
}
Config <.. Host : uses
Config <.. Serialization : uses
Host <.. Serialization : reads attributes
Host <.. Events : reads attributes
Host <.. HostMQ : reads identifiers
Host <.. ApiHost : reads identifiers
Serialization <.. HostMQ : serialize_host
Serialization <.. ApiHost : serialize_host
Events <.. HostMQ : build_event
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
2 times, most recently
from
53f0423 to
3974151
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The new Host.init validation logic appears incorrect:
any(value is not None for value in CANONICAL_FACTS_FIELDS)andall(value is None for value in ID_FACTS)iterate over field-name strings rather than the actual constructor arguments/attributes, so these checks will never behave as intended; consider using the passed-in kwargs orgetattron the instance instead. - In Host.init, the
USE_SUBMAN_IDgate around subscription_manager_id is effectively bypassed because of operator precedence:if current_app.config["USE_SUBMAN_ID"] and (canonical_facts and ... or subscription_manager_id is not None)should be parenthesized so thatUSE_SUBMAN_IDis required even when onlysubscription_manager_idis provided directly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Host.__init__ validation logic appears incorrect: `any(value is not None for value in CANONICAL_FACTS_FIELDS)` and `all(value is None for value in ID_FACTS)` iterate over field-name strings rather than the actual constructor arguments/attributes, so these checks will never behave as intended; consider using the passed-in kwargs or `getattr` on the instance instead.
- In Host.__init__, the `USE_SUBMAN_ID` gate around subscription_manager_id is effectively bypassed because of operator precedence: `if current_app.config["USE_SUBMAN_ID"] and (canonical_facts and ... or subscription_manager_id is not None)` should be parenthesized so that `USE_SUBMAN_ID` is required even when only `subscription_manager_id` is provided directly.
## Individual Comments
### Comment 1
<location> `app/models/host.py:320-321` </location>
<code_context>
groups = []
- if not canonical_facts:
+ # Leaving canonical_facts to avoid changing too much of the code at once
+ if not canonical_facts and not any(value is not None for value in CANONICAL_FACTS_FIELDS):
raise ValidationException("At least one of the canonical fact fields must be present.")
</code_context>
<issue_to_address>
**issue (bug_risk):** Canonical-facts presence check is ineffective because it inspects field names instead of values
`any(value is not None for value in CANONICAL_FACTS_FIELDS)` will always be `True` because it iterates over field-name strings, not host values. This means the `ValidationException` will never be raised for missing canonical facts. To validate correctly, you need to inspect the actual attribute values for those fields (e.g., via `getattr(self, field)` or the constructor args), not the constants themselves.
</issue_to_address>
### Comment 2
<location> `app/models/host.py:324-325` </location>
<code_context>
raise ValidationException("At least one of the canonical fact fields must be present.")
- if all(id_fact not in canonical_facts for id_fact in ID_FACTS):
+ if (canonical_facts and all(id_fact not in canonical_facts for id_fact in ID_FACTS)) or all(
+ value is None for value in ID_FACTS
+ ):
raise ValidationException(f"At least one of the ID fact fields must be present: {ID_FACTS}")
</code_context>
<issue_to_address>
**issue (bug_risk):** ID fact validation also checks field names instead of values, so the `all(... is None ...)` branch is always false
Because `ID_FACTS` is a tuple of constant strings, `all(value is None for value in ID_FACTS)` is always `False`. As a result, when `canonical_facts` is `None`, this condition never passes and the validation doesn’t run, even if all ID-related data is missing. To enforce “at least one ID fact” across both `canonical_facts` and the new columns, this needs to check the actual values (e.g., `canonical_facts.get(...)` and/or the relevant attributes/constructor args), not the constant field names.
</issue_to_address>
### Comment 3
<location> `app/queue/host_mq.py:683-686` </location>
<code_context>
headers = message_headers(
EventType.updated,
- host.canonical_facts.get("insights_id"),
+ str(host.insights_id),
host.reporter,
host.system_profile_facts.get("host_type"),
</code_context>
<issue_to_address>
**issue (bug_risk):** Blindly stringifying insights_id risks propagating "None" instead of null/absence
Several call sites now pass `str(host.insights_id)` (and similar) into headers and cache-key helpers. If `insights_id` is `None`, this becomes the string `'None'`, which is truthy and semantically different from an absent/`None` ID, leading to headers or cache keys literally containing `'None'`. Please avoid stringifying `None` (e.g., use `insights_id and str(insights_id)` or pass `None` through and let downstream code handle it).
</issue_to_address>
### Comment 4
<location> `tests/test_unit.py:987` </location>
<code_context>
expected_errors = HostSchema().validate(inp)
- self.assertEqual(str(expected_errors), str(context.exception))
+ self.assertEqual(str(expected_errors), str(context))
# Test that both of the host schemas will pass all of these fields
</code_context>
<issue_to_address>
**issue (testing):** The invalid-input test is now asserting against the context manager, not the raised exception
Within `with self.assertRaises(ValidationException) as context:`, `context` is the context manager; the raised exception is `context.exception`. The previous assertion compared `str(expected_errors)` to `str(context.exception)`, which correctly checks the exception message. Using `str(context)` instead will not reflect the exception and can make the test pass or fail for the wrong reason. Please compare against `context.exception` (or otherwise explicitly assert on the exception) so the test validates the error correctly.
</issue_to_address>
### Comment 5
<location> `tests/test_models.py:257` </location>
<code_context>
assert error_messages["tags"] == {0: {"key": ["Missing data for required field."]}}
[email protected]("missing_field", ["canonical_facts", "stale_timestamp", "reporter"])
[email protected]("missing_field", ["stale_timestamp", "reporter"])
def test_host_models_missing_fields(missing_field):
limited_values = {
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for Host creation without `canonical_facts` but with canonical fact fields set on the model
This parametrization now reflects that `canonical_facts` is no longer required, but we’re missing a positive test for the new allowed path: creating a Host with `canonical_facts=None` (or omitted) while providing top-level identifiers (e.g. `insights_id`, `subscription_manager_id` when `USE_SUBMAN_ID` is enabled). Please add tests that (1) confirm creation succeeds in that scenario, and (2) confirm creation still fails when both `canonical_facts` and all ID fields are missing/None, ideally colocated with `test_host_models_missing_fields` to keep validation coverage together.
Suggested implementation:
```python
assert error_messages["tags"] == {0: {"key": ["Missing data for required field."]}}
def test_host_creation_with_ids_without_canonical_facts(mocker):
"""
Creating a Host without canonical_facts should succeed as long as
at least one top-level identifier is present (e.g. insights_id or
subscription_manager_id when USE_SUBMAN_ID is enabled).
"""
# NOTE: adapt these to whichever ID fields your Host model actually uses
host_data = {
"account": USER_IDENTITY["account_number"],
"display_name": "host-with-ids-no-cf",
"stale_timestamp": STALE_TIMESTAMP,
"reporter": "test-reporter",
# canonical_facts is intentionally omitted / None
"canonical_facts": None,
"insights_id": "00000000-0000-0000-0000-000000000001",
}
# When USE_SUBMAN_ID is enabled, make sure we also support using that as an identifier
if getattr(settings, "USE_SUBMAN_ID", False):
host_data["subscription_manager_id"] = "subman-id-0001"
host = HostSchema().load(host_data)
# canonical_facts should be allowed to be empty/None, but identifiers should be set
assert host.account == USER_IDENTITY["account_number"]
assert host.display_name == "host-with-ids-no-cf"
assert host.reporter == "test-reporter"
assert host.insights_id == "00000000-0000-0000-0000-000000000001"
if getattr(settings, "USE_SUBMAN_ID", False):
assert host.subscription_manager_id == "subman-id-0001"
# Depending on implementation this may be {} or None; keep the assertion loose
assert host.canonical_facts in ({}, None)
def test_host_creation_fails_without_ids_or_canonical_facts():
"""
Creating a Host must still fail when both canonical_facts and all
top-level identifier fields are missing/None.
"""
host_data = {
"account": USER_IDENTITY["account_number"],
"display_name": "host-without-ids-or-cf",
"stale_timestamp": STALE_TIMESTAMP,
"reporter": "test-reporter",
# canonical_facts explicitly empty / None
"canonical_facts": None,
# All identifier fields omitted / None on purpose
"insights_id": None,
}
if getattr(settings, "USE_SUBMAN_ID", False):
host_data["subscription_manager_id"] = None
with pytest.raises(ValidationError):
HostSchema().load(host_data)
@pytest.mark.parametrize("missing_field", ["stale_timestamp", "reporter"])
```
To make this compile and match your existing conventions, you will likely need to:
1. **Imports**
- Ensure `ValidationError` is imported at the top of `tests/test_models.py`, usually:
`from marshmallow import ValidationError`
- Ensure `settings` and `STALE_TIMESTAMP` are imported/available. If your tests already have a shared timestamp fixture or constant, reuse that instead of `STALE_TIMESTAMP`, or replace with whatever is used in `test_host_models_missing_fields`.
- If you don’t have a `settings` object in tests, replace `getattr(settings, "USE_SUBMAN_ID", False)` with how you normally access the `USE_SUBMAN_ID` flag (e.g. `from app import config` or `from app.environment import USE_SUBMAN_ID`).
2. **Schema / Model names**
- If your schema is not named `HostSchema` or is accessed differently (e.g. `schema.HostSchema()` or `HostSchemaStrict`), adjust the calls accordingly.
- If your `Host` model has different field names for identifiers (e.g. `insights_id`, `satellite_id`, `subscription_manager_id`), update `host_data` and the assertions to match the actual field names.
3. **Fixtures**
- If your tests rely on a DB/session or app fixture (e.g. `db`, `db_session`, `app`), and schema loading requires it, add the appropriate fixture parameters to the test function signatures instead of `mocker` (or remove `mocker` entirely if not needed).
- If `USER_IDENTITY` is provided via a fixture instead of a module-level constant, update the test signatures and usages to accept and use that fixture.
4. **Canonical facts default behavior**
- If your schema normalizes `canonical_facts` to `{}` rather than `None`, you can tighten the assertion to `assert host.canonical_facts == {}` to match the actual behavior.
</issue_to_address>
### Comment 6
<location> `tests/test_unit.py:1902-1905` </location>
<code_context>
self.assertEqual(result, expected)
-class SerializationSerializeCanonicalFactsTestCase(TestCase):
+class SerializationSerializeCanonicalFactsTestCase(FlaskAppTestCase, TestCase):
def test_contains_all_values_unchanged(self):
canonical_facts = {
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen tests for `serialize_canonical_facts` to cover UUID-to-string conversion and host attributes
Since the tests now pass a `Host` instance into `serialize_canonical_facts`, please add a test that builds a `Host` with one or more canonical-fact fields as actual `UUID` objects (e.g., `insights_id`, `subscription_manager_id`, or `openshift_cluster_id`) and asserts that the serialized output contains those fields as strings. This will exercise the UUID-to-string conversion path and help prevent UUID objects from leaking into MQ payloads.
Suggested implementation:
```python
class SerializationSerializeCanonicalFactsTestCase(FlaskAppTestCase, TestCase):
def test_uuid_canonical_facts_are_serialized_as_strings(self):
insights_id = uuid4()
subscription_manager_id = uuid4()
openshift_cluster_id = uuid4()
host = Host(
insights_id=insights_id,
subscription_manager_id=subscription_manager_id,
openshift_cluster_id=openshift_cluster_id,
display_name="uuid-test-host",
stale_timestamp=now(),
reporter="test",
)
serialized = serialize_canonical_facts(host)
self.assertEqual(serialized["insights_id"], str(insights_id))
self.assertEqual(serialized["subscription_manager_id"], str(subscription_manager_id))
self.assertEqual(serialized["openshift_cluster_id"], str(openshift_cluster_id))
self.assertIsInstance(serialized["insights_id"], str)
self.assertIsInstance(serialized["subscription_manager_id"], str)
self.assertIsInstance(serialized["openshift_cluster_id"], str)
```
1. Ensure that `serialize_canonical_facts` is imported into `tests/test_unit.py` if it is not already, e.g.:
`from app.serialization import serialize_canonical_facts` (adjust the module path to match your project).
2. `Host`, `now`, `FlaskAppTestCase`, and `uuid4` appear to already be in use in this file; if any are missing, add the appropriate imports consistent with existing conventions.
</issue_to_address>
### Comment 7
<location> `tests/test_host_mq_service.py:3060-3062` </location>
<code_context>
id = "host-id"
org_id = "org-id"
account = "acct"
- canonical_facts = {"insights_id": str(generate_uuid())}
+ canonical_facts = {"insights_id": str(generate_uuid())} # this line will be removed
+ insights_id = generate_uuid()
+ subscription_manager_id = generate_uuid()
reporter = "puptoo"
system_profile_facts = {"owner_id": "owner-id"}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to cover `_set_owner` behavior when using `subscription_manager_id` instead of `canonical_facts`
The MQ path in `host_mq.py` now derives `owner_id` from `host.subscription_manager_id` for certain reporters, but there’s no end-to-end test validating this behavior.
Please add tests (likely alongside the existing MQ owner tests) that:
- Use a host with `subscription_manager_id` set, no `canonical_facts` dependency, and a reporter of `"rhsm-conduit"` or `"rhsm-system-profile-bridge"`, then assert `system_profile_facts["owner_id"]` (and static profile, if used) matches the formatted `subscription_manager_id` after `_set_owner` runs.
- Cover the case where `subscription_manager_id` is missing but identity contains an owner, and confirm that a `ValidationException` is still raised on mismatch.
This will ensure the refactor away from `canonical_facts` preserves owner assignment semantics.
Suggested implementation:
```python
id = "host-id"
org_id = "org-id"
account = "acct"
insights_id = generate_uuid()
subscription_manager_id = generate_uuid()
reporter = "puptoo"
system_profile_facts = {"owner_id": "owner-id"}
groups = [serialized_group]
```
To fully implement the test coverage requested in your review comment, you will also need to:
1. **Add an end-to-end MQ test for `subscription_manager_id` owner assignment (success case)**
Place this near the existing MQ owner-related tests (e.g., close to tests that currently verify `_set_owner` behavior based on `canonical_facts` or identity).
The new test should:
- Construct a host payload with:
- `subscription_manager_id = generate_uuid()`
- `insights_id` set (if required by the payload schema)
- No dependency on `canonical_facts` for determining the owner (i.e., the test should not set a `canonical_facts["insights_id"]` that is used by `_set_owner`).
- `reporter` equal to `"rhsm-conduit"` (and another test for `"rhsm-system-profile-bridge"` if behavior is identical but you want explicit coverage).
- Call the same helper / path that current MQ tests use to send a message through `host_mq` (e.g., something like `emit_event` -> handler -> `_set_owner`), so `_set_owner` is exercised in the same way as in production.
- Assert that:
- `system_profile_facts["owner_id"]` on the resulting host matches the formatted `subscription_manager_id` (whatever transformation `_set_owner` currently applies).
- If the static system profile is persisted separately in your tests (e.g., `host.system_profile_facts` or a separate static profile store), assert it is consistent with the formatted `subscription_manager_id`.
A skeleton that you will need to adjust to your test utilities and models might look like:
```python
def test_mq_set_owner_from_subscription_manager_id_rhsm_conduit(
mq_test_client,
db_session,
generate_uuid,
identity_header_factory,
):
subscription_manager_id = generate_uuid()
host_payload = {
"id": "host-id",
"org_id": "org-id",
"account": "acct",
"insights_id": str(generate_uuid()),
"subscription_manager_id": str(subscription_manager_id),
"reporter": "rhsm-conduit",
"system_profile": {
# owner_id will be overridden by _set_owner
"owner_id": "pre-existing-owner",
},
}
identity = identity_header_factory(account_number="acct", org_id="org-id")
# Use whatever helper you have that goes through the MQ flow and `_set_owner`
created_host = send_mq_host_and_get_created_host(
mq_test_client, host_payload, identity
)
expected_owner = format_subscription_manager_id(subscription_manager_id)
assert created_host.system_profile_facts["owner_id"] == expected_owner
# If you have a second representation (static stored profile), assert that too.
```
Then duplicate / parametrize this test for `"rhsm-system-profile-bridge"` as the reporter.
2. **Add a test for the mismatch ValidationException with subscription_manager_id missing but identity owner present**
Near the existing tests that confirm `_set_owner` raises `ValidationException` when identity owner mismatches what’s on the host, add a new test that:
- Constructs a host payload **without** `subscription_manager_id`.
- Uses an identity whose owner is set (e.g., an `x-rh-identity` with an `entitlements` owner or something equivalent in your code base).
- Sets `system_profile_facts["owner_id"]` on the host payload such that it conflicts with the identity’s owner.
- Sends the host through the same MQ path that triggers `_set_owner`.
- Asserts that a `ValidationException` is raised (using `pytest.raises(ValidationException)` or your existing pattern).
Example skeleton (adapt names to match your code):
```python
def test_mq_owner_mismatch_without_subscription_manager_id_raises_validation(
mq_test_client,
db_session,
identity_header_factory,
):
conflicting_owner = "owner-from-host"
identity_owner = "owner-from-identity"
host_payload = {
"id": "host-id",
"org_id": "org-id",
"account": "acct",
"insights_id": str(generate_uuid()),
# subscription_manager_id intentionally omitted
"reporter": "rhsm-conduit",
"system_profile": {
"owner_id": conflicting_owner,
},
}
identity = identity_header_factory(
account_number="acct",
org_id="org-id",
owner=identity_owner,
)
with pytest.raises(ValidationException):
send_mq_host_and_get_created_host(
mq_test_client, host_payload, identity
)
```
3. **Reuse existing helpers and conventions**
- Replace `send_mq_host_and_get_created_host`, `format_subscription_manager_id`, and any other placeholder helpers in the skeletons with the actual functions/fixtures used elsewhere in `tests/test_host_mq_service.py` for MQ flows and owner tests.
- Ensure imports (`pytest`, `ValidationException`, any MQ client fixtures, identity helpers, etc.) are consistent with the rest of the file. If similar tests already import `ValidationException` or identity factories, reuse those imports and fixtures.
With these additional tests in place, you will have explicit coverage for `_set_owner` behavior when using `subscription_manager_id` and when falling back to identity with `subscription_manager_id` missing, including the mismatch error path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
from
3974151 to
cf8f588
Compare
Contributor
Author
|
/retest |
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
7 times, most recently
from
96c42af to
2395cbe
Compare
Contributor
Author
|
/retest |
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
5 times, most recently
from
970428d to
f213120
Compare
Contributor
Author
|
/retest |
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
from
0e95198 to
8a034a2
Compare
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
ezr-ondrej
previously approved these changes
Dec 22, 2025
Member
ezr-ondrej
left a comment
ezr-ondrej
left a comment
There was a problem hiding this comment.
LGTM 👍 Let see what Fero thinks :)
Contributor
Author
|
I'll update the branch once all tests pass |
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
from
8a034a2 to
74fe726
Compare
Member
|
@FabriciaDinizRH The changes look good, you will need to update the iqe test: |
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
3 times, most recently
from
39029b5 to
f614bf2
Compare
refactor(RHINENG-22723): remove canonical_facts from MQ
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
from
f614bf2 to
5de048c
Compare
Contributor
Author
|
/retest |
2 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
fstavela
requested changes
Dec 23, 2025
Contributor
fstavela
left a comment
fstavela
left a comment
There was a problem hiding this comment.
Thank you for working on this. I left a few comments/questions, please take a look.
| "context": { | ||
| "inventory_id": host.get("id"), | ||
| "hostname": canonical_facts.get("fqdn", ""), | ||
| "hostname": host.get("fqdn") or "", |
Contributor
There was a problem hiding this comment.
Optional:
Suggested change
| "hostname": host.get("fqdn") or "", | |
| "hostname": host.get("fqdn", ""), |
Contributor
Author
There was a problem hiding this comment.
@fstavela for some reason this breaks IQE notification tests, I'll undo this change
Contributor
Author
There was a problem hiding this comment.
it's a situation similar to the insights_id
Contributor
There was a problem hiding this comment.
Hmm, this is strange, the two statements should be exactly the same 🤔
Contributor
There was a problem hiding this comment.
I'm going to approve, as I think it doesn't really matter, but it's possible that the failure was caused just by some flakiness, because I don't know how this could affect anything.
Contributor
Author
|
@fstavela before we were getting the insights_id from the canonical facts, and it already was a string. Now we're getting it directly from the host, where it is stored as UUID. Since insights_id is not supposed to be |
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
from
2ebf481 to
48e7e69
Compare
Contributor
Thank you for the explanation! |
Co-authored-by: František Viktor Stavěla <[email protected]>
FabriciaDinizRH
force-pushed
the
remove-cf-from-mq
branch
from
48e7e69 to
b3f23af
Compare
fstavela
previously approved these changes
Dec 23, 2025
fstavela
approved these changes
Dec 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Overview
This PR is being created to address RHINENG-22723.
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Refactor host messaging and serialization to stop relying on the legacy canonical_facts dict and instead use dedicated Host fields for MQ events and related logic.
Enhancements:
Tests:
Summary by Sourcery
Refactor host canonical facts handling to rely on dedicated Host model fields instead of the legacy canonical_facts dict, updating serialization, MQ event production, and tests accordingly.
Enhancements:
Tests: