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.

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

Clear suma discoveries #2394

Merged
merged 5 commits into from
Mar 12, 2024
Merged

Clear suma discoveries #2394

merged 5 commits into from
Mar 12, 2024

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Mar 7, 2024

Description

This PR introduces the ability to clear up previously discovered software updates information when software updates settings get cleared themselves.

Mechanism is pretty simple:

  1. attempt to clear software updates settings
  2. for each registered* host issue a ClearSoftwareUpdatesDiscovery** -> host ignores previously discovered info
  3. the state of the integration service is cleared up as well

* wondering whether it is enough and if we should do anything on host deregistration to wipe out software updates discovery. Deferring.

** current implementation works in a best effort fashion, meaning that failures when dispatching ClearSoftwareUpdatesDiscovery do not block the overall clear up process.

How was this tested?

Automated tests.

Note that a couple of test cases have been added:

  • Trento.CommandedCase
  • Trento.SoftwareUpdates.DiscoveryCase

These provide respectively a default stub for commanded dispatching and for the software updates discovery.
They come handy when a default behaviour is enough in tests and no specific expectation is needed for respective behaviours.

@nelsonkopliku nelsonkopliku changed the title Add a clear function to Discovery contract Clear suma discoveries Mar 7, 2024
@nelsonkopliku nelsonkopliku self-assigned this Mar 7, 2024
@nelsonkopliku nelsonkopliku force-pushed the clear-suma-discovery-results branch 4 times, most recently from 860ef21 to 79d317e Compare March 12, 2024 08:03
@nelsonkopliku nelsonkopliku marked this pull request as ready for review March 12, 2024 08:06
@nelsonkopliku nelsonkopliku added enhancement New feature or request user story elixir Pull requests that update Elixir code labels Mar 12, 2024
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Just a couple minor nitpicks but as Chester Bennington taught us in the time of our lives, in the end it doesn't even matter

lib/trento/software_updates.ex Outdated Show resolved Hide resolved
Comment on lines 48 to 52
Enum.each(hosts, fn %HostReadModel{id: host_id} ->
%{host_id: host_id}
|> ClearSoftwareUpdatesDiscovery.new!()
|> commanded().dispatch()
end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you move this outside the if clause? You could just use Enum.map to build the commands and check the length of that list as well, but really up to you, it's a nitpick

Copy link
Member Author

@nelsonkopliku nelsonkopliku Mar 12, 2024

Choose a reason for hiding this comment

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

pick one

def clear_software_updates_discoveries do
  hosts = Hosts.get_all_hosts()

  Enum.each(hosts, fn %HostReadModel{id: host_id} ->
    %{host_id: host_id}
    |> ClearSoftwareUpdatesDiscovery.new!()
    |> commanded().dispatch()
  end)

  if length(hosts) > 0 do # or  if !Enum.empty?(hosts) do
    clear()
  end

  :ok
end
def clear_software_updates_discoveries do
  hosts = Hosts.get_all_hosts()

  hosts
  |> Enum.map(fn %HostReadModel{id: host_id} -> %{host_id: host_id} end)
  |> Enum.each(fn command_payload ->
    command_payload
    |> ClearSoftwareUpdatesDiscovery.new!()
    |> commanded().dispatch()
  end)

  if length(hosts) > 0 do # or  if !Enum.empty?(hosts) do
    clear()
  end

  :ok
end

Copy link
Contributor

Choose a reason for hiding this comment

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

The second one monsieur 👨‍🍳 (chef kiss emoji sadly unavailable)

Copy link
Member Author

@nelsonkopliku nelsonkopliku Mar 12, 2024

Choose a reason for hiding this comment

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

I am fine eitherway.
I like the second option more, however it is less efficient as we iterate over items twice, but hey 😄

EDIT: Big O forgive me :trollface:

@nelsonkopliku nelsonkopliku force-pushed the clear-suma-discovery-results branch from 79d317e to 63eb689 Compare March 12, 2024 15:34
@dottorblaster
Copy link
Contributor

image

@nelsonkopliku nelsonkopliku merged commit 38c28b0 into main Mar 12, 2024
24 checks passed
@nelsonkopliku nelsonkopliku deleted the clear-suma-discovery-results branch March 12, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or request user story
Development

Successfully merging this pull request may close these issues.

3 participants