Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add installation_source field to the host telemetry #728

Merged
merged 6 commits into from Sep 12, 2022
Merged

Add installation_source field to the host telemetry #728

merged 6 commits into from Sep 12, 2022

Conversation

arbulu89
Copy link
Contributor

Add the installation_source field to the host telemetry model.

This field is not required in the agent side, we so it is still backwards compatible with older agent version. In this cases, it is populated with Unknown string.

Depends on: trento-project/agent#64

@arbulu89 arbulu89 added the enhancement New feature or request label Jul 29, 2022
@arbulu89 arbulu89 marked this pull request as ready for review August 1, 2022 07:59
@arbulu89 arbulu89 requested a review from CDimonaco August 1, 2022 07:59
rtorrero
rtorrero approved these changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had imaged here he would use something like the repo url to identify the installation source, but I guess that what you propose here is more flexible. Looks good to me @arbulu89, thanks!

fabriziosestito
fabriziosestito suggested changes Aug 1, 2022
View reviewed changes
lib/trento/application/integration/discovery/payloads/host_discovery_payload.ex Outdated
@@ -16,5 +25,6 @@ defmodule Trento.Integration.Discovery.HostDiscoveryPayload do
field :total_memory_mb, :integer
field :socket_count, :integer
field :os_version, :string
field :installation_source, :string, default: "Unknown"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we limit the possible values here and validate them against a set of values aka :unknown, :community, :premium?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in: 6f6c1c7

lib/trento/application/read_models/host_telemetry_read_model.ex Outdated
@@ -18,6 +18,7 @@ defmodule Trento.HostTelemetryReadModel do
field :socket_count, :integer
field :total_memory_mb, :integer
field :sles_version, :string
field :installation_source, :string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ecto.Enum here with fixed values as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 6f6c1c7

@arbulu89
Copy link
Contributor Author

arbulu89 commented Aug 2, 2022

Moving to Draft as this change creates a risky tech debt as the event store is not properly upcasted

@arbulu89 arbulu89 marked this pull request as draft August 2, 2022 09:36
@fabriziosestito
Copy link
Member

fabriziosestito commented Sep 6, 2022 •
edited

@arbulu89 Unblocked by #751

@arbulu89 arbulu89 mentioned this pull request Sep 6, 2022
Merged
@arbulu89 arbulu89 force-pushed the add-installation-source branch 2 times, most recently from a0e0702 to 99c7260 Compare September 6, 2022 13:05
@arbulu89
Copy link
Contributor Author

arbulu89 commented Sep 6, 2022

Upcasting applied. Waiting for #751 to be rebased

@arbulu89 arbulu89 marked this pull request as ready for review September 6, 2022 14:32
@arbulu89
Copy link
Contributor Author

arbulu89 commented Sep 6, 2022

Hey @fabriziosestito ,
The upcasting implemented ea9ddb9. Finally I have added a couple of basic tests, as they are quick to write.

fabriziosestito
fabriziosestito reviewed Sep 12, 2022
View reviewed changes
test/trento/domain/host/host_test.exs Outdated
%{}
|> HostDetailsUpdated.upcast(%{})
|> HostDetailsUpdated.new!()
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split these tests into two different files with the event module name to reflect the module structure (e.g. domain/hosts/events/host_details_updated.exs`

Also, the test is a bit unclear, we need to upcast a map that is at version 1, not an empty map because this is what we get from the event store when dealing with old events.

Something like

%{
                 "version" => 1,
# insert all the host details updated events fields that are present in version 1 possibly with a faker
....
               }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or event without the version field, since it should default it at version 1

fabriziosestito
fabriziosestito suggested changes Sep 12, 2022
View reviewed changes
Copy link
Member

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor stuff then good to go

@arbulu89
Copy link
Contributor Author

Some minor stuff then good to go

Thanks @fabriziosestito ,
Changes done!

@fabriziosestito
Copy link
Member

Some minor stuff then good to go

Thanks @fabriziosestito , Changes done!

@arbulu89 go!

@arbulu89 arbulu89 merged commit 824751d into main Sep 12, 2022
@arbulu89 arbulu89 deleted the add-installation-source branch September 12, 2022 08:06
@arbulu89 arbulu89 mentioned this pull request Sep 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziosestito fabriziosestito fabriziosestito approved these changes

@rtorrero rtorrero rtorrero approved these changes

@dottorblaster dottorblaster Awaiting requested review from dottorblaster

@CDimonaco CDimonaco Awaiting requested review from CDimonaco

Assignees
No one assigned
Labels
enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arbulu89 @fabriziosestito @rtorrero