RHINENG-22325: inventory views #2014
Open
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
|
Referenced Jiras: |
Reviewer's GuideAdds inventory views Kafka event publishing from the evaluator, introduces a shared TemplateBase model, and extends the internal Kafka messaging abstraction to support headers and a new inventory views topic across configuration and deployment artifacts. Sequence diagram for publishing inventory views events from evaluatorsequenceDiagram
participant Eval as evaluator
participant InvViews as inventory_views
participant Mq as mqueue
participant Kafka as kafka_topic_platform_inventory_host_apps
participant DB as db
Eval->>Eval: evaluateAndStore(system, event)
alt inventory_views_enabled
Eval->>Eval: publishInventoryViewsEvent(tx, [system], event)
Eval->>InvViews: MakeInventoryViewsEvent(tx, orgID, systems)
InvViews->>InvViews: FindSystemsTemplates(tx, systems)
InvViews->>DB: query TemplateBase by rh_account_id and id
DB-->>InvViews: templates map
InvViews->>InvViews: MakeInventoryViewsHosts(systems, templates)
InvViews-->>Eval: InventoryViewsEvent
Eval->>Mq: MessageFromJSON(orgID, InventoryViewsEvent, headers)
Mq-->>Eval: KafkaMessage(key, value, headers)
Eval->>Mq: inventoryViewsPublisher.WriteMessages(ctx, KafkaMessage)
Mq->>Kafka: send message with key, value, headers
Kafka-->>Mq: ack
Mq-->>Eval: nil error
else publisher_not_configured
Eval-->>Eval: publishInventoryViewsEvent returns without sending
end
ER diagram for template and template_base usage in inventory viewserDiagram
template_base {
int64 id PK
int rh_account_id PK
string uuid
string name
}
template {
int64 id PK
int rh_account_id PK
string uuid
string name
string environment_id
string arch
string version
time created_at
time updated_at
time last_edited
}
system_platform {
int64 id PK
int rh_account_id
string inventory_id
int64 template_id FK
}
template_base ||--o{ template : is_embedded_in
template_base ||--o{ system_platform : referenced_by
template ||--|| template_base : shares_primary_key
Updated class diagram for templates, inventory views, and Kafka messagingclassDiagram
class TemplateBase {
+int64 ID
+int RhAccountID
+string UUID
+string Name
+TableName() string
}
class Template {
+string EnvironmentID
+string Arch
+string Version
+time.Time CreatedAt
+time.Time UpdatedAt
+time.Time LastEdited
+TableName() string
}
TemplateBase <|-- Template
class SystemPlatform {
+int64 ID
+int RhAccountID
+string InventoryID
+*int64 TemplateID
+int ApplicableAdvisorySecCountCache
+int ApplicableAdvisoryBugCountCache
+int ApplicableAdvisoryEnhCountCache
+int ApplicableAdvisoryCountCache
+int InstallableAdvisorySecCountCache
+int InstallableAdvisoryBugCountCache
+int InstallableAdvisoryEnhCountCache
+int InstallableAdvisoryCountCache
+int PackagesInstalled
+int PackagesInstallable
+int PackagesApplicable
}
TemplateBase <.. SystemPlatform : used_for_template_lookup
class InventoryViewsHostData {
+int ApplicableRhsaCount
+int ApplicableRhbaCount
+int ApplicableRheaCount
+int ApplicableOtherCount
+int InstallableRhsaCount
+int InstallableRhbaCount
+int InstallableRheaCount
+int InstallableOtherCount
+int PackagesInstalled
+int PackagesInstallable
+int PackagesApplicable
+*string TemplateName
+*string TemplateUUID
}
class InventoryViewsHost {
+string ID
+InventoryViewsHostData Data
}
class InventoryViewsEvent {
+string OrgID
+string Timestamp
+[]InventoryViewsHost Hosts
+MakeInventoryViewsEvent(tx *gorm.DB, orgID string, systems []SystemPlatform) (InventoryViewsEvent, error)
+MakeInventoryViewsHosts(systems []SystemPlatform, templates map[int64]TemplateBase) ([]InventoryViewsHost, error)
+FindSystemsTemplates(tx *gorm.DB, systems []SystemPlatform) (map[int64]TemplateBase, error)
}
InventoryViewsHostData <.. InventoryViewsHost : contains
InventoryViewsHost <.. InventoryViewsEvent : aggregates
class KafkaMessage {
+[]byte Key
+[]byte Value
+[]kafka.Header Headers
}
class mqueue {
+MessageFromJSON(key string, value interface, headers []kafka.Header) (KafkaMessage, error)
+Writer
}
class kafkaGoReaderImpl {
-*kafka.Reader Reader
+HandleMessages(handler MessageHandler)
}
class kafkaGoWriterImpl {
-*kafka.Writer Writer
+WriteMessages(ctx context.Context, msgs ...KafkaMessage) error
}
KafkaMessage <.. mqueue : created_by
KafkaMessage <.. kafkaGoWriterImpl : converted_to_kafka_Message
KafkaMessage <.. kafkaGoReaderImpl : constructed_from_kafka_Message
class Evaluator {
-bool enableInventoryViews
+configureInventoryViews()
+publishInventoryViewsEvent(tx *gorm.DB, systems []SystemPlatform, origin *mqueue.PlatformEvent) error
}
class CoreCfg {
+string InventoryViewsTopic
+string KafkaAddress
+[]string KafkaServers
}
Evaluator ..> InventoryViewsEvent : publishes
Evaluator ..> KafkaMessage : sends
Evaluator ..> CoreCfg : reads_configuration
CoreCfg ..> mqueue : new_kafka_writer
kafkaGoWriterImpl ..|> mqueue.Writer
kafkaGoReaderImpl ..|> mqueue.Reader
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2014 +/- ##
==========================================
+ Coverage 59.01% 59.18% +0.16%
==========================================
Files 131 133 +2
Lines 8493 8599 +106
==========================================
+ Hits 5012 5089 +77
- Misses 2947 2967 +20
- Partials 534 543 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MichaelMraka
force-pushed
the
pr1
branch
3 times, most recently
from
b4121ec to
158785f
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
MakeInventoryViewsHosts, when a system has aTemplateIDthat isn't present in thetemplatesmap, you log a warning but still unconditionally setTemplateName/TemplateUUIDfrom the zero-valuetemplatevariable; this yields non-nil empty strings instead ofnil, which contradicts the test expectations and should be guarded by theokflag. MakeInventoryViewsHostsreturns([]InventoryViewsHost, error)but never actually produces a non-nil error; consider simplifying the signature to drop theerrorreturn or add concrete error conditions to make the return type meaningful.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MakeInventoryViewsHosts`, when a system has a `TemplateID` that isn't present in the `templates` map, you log a warning but still unconditionally set `TemplateName`/`TemplateUUID` from the zero-value `template` variable; this yields non-nil empty strings instead of `nil`, which contradicts the test expectations and should be guarded by the `ok` flag.
- `MakeInventoryViewsHosts` returns `([]InventoryViewsHost, error)` but never actually produces a non-nil error; consider simplifying the signature to drop the `error` return or add concrete error conditions to make the return type meaningful.
## Individual Comments
### Comment 1
<location> `base/inventory_views/inventory_views_test.go:68-77` </location>
<code_context>
+func TestMakeInventoryViewsEventNoTemplate(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for systems that reference a non-existent template row
Current tests cover valid templates, an empty systems slice, and `TemplateID == nil`. There’s still an untested path where `TemplateID != nil` but `FindSystemsTemplates` returns no row (e.g. deleted template): we log a warning, but the resulting `TemplateName`/`TemplateUUID` behavior isn’t asserted. Please add a DB-backed test that sets `TemplateID` to a non-existent template ID and verifies that `MakeInventoryViewsEvent` does not error, and that `TemplateName`/`TemplateUUID` match the intended behavior for dangling template references.
Suggested implementation:
```golang
func TestMakeInventoryViewsEventNoTemplate(t *testing.T) {
utils.SkipWithoutDB(t)
core.SetupTestEnvironment()
tx := database.DB.Begin()
defer tx.Rollback()
var rhAccount models.RhAccount
assert.NoError(t, tx.Where("id = ?", rhAccountID).First(&rhAccount).Error)
assert.NotEmpty(t, *rhAccount.OrgID)
orgID := *rhAccount.OrgID
// Use a template ID that does not exist in the templates table to simulate
// a dangling template reference (e.g. the template was deleted).
var nonExistentTemplateID uint = 999999999
system := models.SystemPlatform{
RhAccountID: &rhAccount.ID,
OrgID: &orgID,
TemplateID: &nonExistentTemplateID,
}
event, err := MakeInventoryViewsEvent(tx, orgID, []models.SystemPlatform{system})
assert.NoError(t, err)
assert.Equal(t, orgID, event.OrgID)
assert.Len(t, event.Hosts, 1)
host := event.Hosts[0]
// When a system references a non-existent template row, we only log a warning
// and still emit the host; template-related fields should reflect the
// "missing template" behavior (no template data attached).
assert.Zero(t, host.TemplateUUID)
assert.Zero(t, host.TemplateName)
}
```
1. Ensure that `models.SystemPlatform` has the fields `RhAccountID`, `OrgID`, and `TemplateID` with compatible types:
- `RhAccountID` should match the type of `rhAccount.ID` (commonly `uint` or `int64`) and be a pointer if used as `&rhAccount.ID`.
- `OrgID` should be a `*string` to match `orgID := *rhAccount.OrgID`.
- `TemplateID` should be a pointer to an integer type (here `*uint`). If it is a different type (e.g. `*int64` or `*uuid.UUID`), adjust the definition of `nonExistentTemplateID` and the field type accordingly.
2. Confirm the `event.Hosts` element type exposes `TemplateUUID` and `TemplateName` fields. If their names or types differ:
- Update `host.TemplateUUID` / `host.TemplateName` to the correct field names.
- Keep the `assert.Zero` checks so the test remains agnostic to whether the fields are strings or pointers; `assert.Zero` will pass for `""` or `nil`.
3. If `MakeInventoryViewsEvent` is expected to behave differently for dangling template references (e.g. keep a UUID but clear only the name), adjust the assertions on `host.TemplateUUID` / `host.TemplateName` to match that intended behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Add support for publishing inventory views Kafka events from the evaluator, including associated configuration, topic setup, and data model utilities.
New Features:
Enhancements:
Build:
Deployment:
Tests: