-
Notifications
You must be signed in to change notification settings - Fork 92
refactor(RHIENG-22722): remove canonical_facts from API #3288
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
base: master
Are you sure you want to change the base?
refactor(RHIENG-22722): remove canonical_facts from API #3288
Conversation
Reviewer's GuideRefactors host canonical facts handling by removing the Host.canonical_facts JSON field from the API/model surface and tests, instead flowing canonical fact values as top-level fields, constructing canonical facts internally from those columns, and updating serialization, deserialization, matching, and event emission logic accordingly. Sequence diagram for updated host deserialization and constructionsequenceDiagram
actor Client
participant API_host as API_host_endpoint
participant Serialization as app_serialization
participant Schema as HostSchema
participant Host as Host_model
Client->>API_host: POST /hosts body
API_host->>Serialization: deserialize_host(raw_data, schema)
Serialization->>Serialization: deserialize_canonical_facts(raw_data)
Serialization->>Serialization: _deserialize_facts(raw_data.facts)
Serialization->>Serialization: _deserialize_tags(raw_data.tags)
Note over Serialization: Build data dict containing both
Note over Serialization: payload fields and canonical facts columns
Serialization->>Schema: build_model(validated_data_with_cf, facts, tags, tags_alt)
Schema->>Host: __init__(canonical_facts=None, display_name, ansible_host, account, org_id, facts, tags, tags_alt, system_profile_facts, stale_timestamp, reporter, groups, insights_id, subscription_manager_id, satellite_id, fqdn, bios_uuid, ip_addresses, mac_addresses, provider_id, provider_type, openshift_cluster_id, per_reporter_staleness)
Note over Host: Construct constructed_canonical_facts from
Note over Host: individual canonical fact attributes
Host->>Host: update_canonical_facts_columns(constructed_canonical_facts)
Host-->>Schema: Host instance
Schema-->>Serialization: Host instance
Serialization-->>API_host: Host instance
API_host-->>Client: HTTP 201 with serialized host
Sequence diagram for add_host matching using extracted canonical factssequenceDiagram
actor Service as Caller_service
participant HostRepo as host_repository
participant Extract as extract_canonical_facts_from_host
participant Finder as find_existing_host
participant DB as database
Service->>HostRepo: add_host(input_host, existing_hosts, identity, operation_args)
HostRepo->>Extract: extract_canonical_facts_from_host(input_host)
Extract-->>HostRepo: input_host_canonical_facts
alt existing_hosts provided
HostRepo->>Finder: find_existing_host(identity, input_host_canonical_facts, existing_hosts)
Finder-->>HostRepo: matched_host or None
end
alt matched_host is None
HostRepo->>Finder: find_existing_host(identity, input_host_canonical_facts)
Finder->>DB: query hosts by canonical fact columns
DB-->>Finder: existing host or None
Finder-->>HostRepo: matched_host or None
end
Note over HostRepo: Canonical facts are now matched
Note over HostRepo: via top level columns, not host.canonical_facts JSON
HostRepo->>DB: insert or update Host row
DB-->>HostRepo: persisted Host
HostRepo-->>Service: resulting Host
Class diagram for updated Host canonical facts handlingclassDiagram
class Host {
+UUID id
+str account
+str org_id
+str display_name
+str ansible_host
+dict facts
+list tags
+list tags_alt
+dict system_profile_facts
+datetime stale_timestamp
+str reporter
+list groups
+datetime last_check_in
+UUID insights_id
+str subscription_manager_id
+str satellite_id
+str fqdn
+str bios_uuid
+list ip_addresses
+list mac_addresses
+str provider_id
+str provider_type
+str openshift_cluster_id
+dict per_reporter_staleness
+__init__(canonical_facts, display_name, ansible_host, account, org_id, facts, tags, tags_alt, system_profile_facts, stale_timestamp, reporter, groups, insights_id, subscription_manager_id, satellite_id, fqdn, bios_uuid, ip_addresses, mac_addresses, provider_id, provider_type, openshift_cluster_id, per_reporter_staleness)
+update(input_host, update_system_profile)
+update_display_name(display_name, reporter, input_fqdn)
+update_canonical_facts_columns(host)
+save()
}
class LimitedHost {
+UUID id
+str account
+str org_id
+str display_name
+str ansible_host
+dict facts
+list tags
+list tags_alt
+dict system_profile_facts
+list groups
+UUID insights_id
+str subscription_manager_id
+str satellite_id
+str fqdn
+str bios_uuid
+list ip_addresses
+list mac_addresses
+str provider_id
+str provider_type
+str openshift_cluster_id
+__init__(display_name, ansible_host, account, org_id, facts, tags, tags_alt, system_profile_facts, groups, insights_id, subscription_manager_id, satellite_id, fqdn, bios_uuid, ip_addresses, mac_addresses, provider_id, provider_type, openshift_cluster_id)
}
class HostSchema {
+build_model(data, facts, tags, tags_alt)
}
class LimitedHostSchema {
+build_model(data, facts, tags, tags_alt)
}
class Config {
+tuple CANONICAL_FACTS_FIELDS
}
class Serialization {
+deserialize_host(raw_data, schema)
+deserialize_canonical_facts(raw_data, all)
+_deserialize_canonical_facts(data)
+_deserialize_all_canonical_facts(data)
+serialize_host(host, timestamps, for_mq, additional_fields, staleness, system_profile_fields)
+serialize_canonical_facts(canonical_facts)
+extract_canonical_facts_from_host(host)
}
Host <|-- LimitedHost
HostSchema --> Host
LimitedHostSchema --> LimitedHost
Serialization --> HostSchema
Serialization --> LimitedHostSchema
Host ..> Config : uses CANONICAL_FACTS_FIELDS
Serialization ..> Config : uses CANONICAL_FACTS_FIELDS
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ezr-ondrej
left a comment
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.
I've done a quick pass, didn't review the more complex changes yet :)
| if current_app.config["USE_SUBMAN_ID"] and "subscription_manager_id" in constructed_canonical_facts: | ||
| id = constructed_canonical_facts["subscription_manager_id"] |
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.
| if current_app.config["USE_SUBMAN_ID"] and "subscription_manager_id" in constructed_canonical_facts: | |
| id = constructed_canonical_facts["subscription_manager_id"] | |
| if current_app.config["USE_SUBMAN_ID"] and subscription_manager_id: | |
| id = subscription_manager_id |
|
|
||
| self.update_canonical_facts(canonical_facts) | ||
| self.update_canonical_facts_columns(canonical_facts) | ||
| self.update_canonical_facts_columns(constructed_canonical_facts) |
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.
I think this is no longer needed, because the fields are no longer settable from canonical facts, only directly, so there is no need to sync them from canonical facts
|
|
||
| def __repr__(self): | ||
| return ( | ||
| f"<Host id='{self.id}' account='{self.account}' org_id='{self.org_id}' display_name='{self.display_name}' " |
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.
It might be good to add at least subman_id, or directly all CANONICAL_FACTS_FIELDS:
| f"<Host id='{self.id}' account='{self.account}' org_id='{self.org_id}' display_name='{self.display_name}' " | |
| f"<Host id='{self.id}' account='{self.account}' org_id='{self.org_id}' display_name='{self.display_name}' " | |
| f"{[key + '=' + getattr(key) for key in CANONICAL_FACTS_FIELD].join(' ')} >" |
At minimum, you're missing the closing bracket (>), that was part of the removed line
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
Remove usage of the host.canonical_facts field in favor of first-class canonical fact columns and update APIs, serialization, and repository logic accordingly.
Enhancements:
Tests: