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

Fixes #36104 - Distinguish host events #9780

Merged

Conversation

ofedoren
Copy link
Member

Testable with foreman_webhooks. Adds new events to better distinguish between host events, whether it was triggered by user or via some automation.

@adamruzicka, what do you think? This is the simplest solution I could find and it seems to solve the issue...

@theforeman-bot
Copy link
Member

Issues: #36104

@ofedoren
Copy link
Member Author

Added a second commit, which removes new host_*[_automatically|_manually] events. Host created/destroyed can be left as general events, since they are pretty rare in comparison to host updated event we want to resolve problems with (due to often auto updates via e.g. reports). This commit ensures host_updated event is only triggered on actual user input against the host object. Auto updates (such as reports or changes through console) won't or shouldn't trigger this event. This comes from offline discussion with @ares, but any other input is surely welcome.

@adamruzicka
Copy link
Contributor

I like this, it is nice and clean and seems to work well. However, shouldn't there be an event that would fire once uploaded facts get processed?

@ofedoren
Copy link
Member Author

@adamruzicka, that event can cause spammed host_updated events. If we drop the second commit then it will fire host_updated_automatically event, which nobody used before, so it's pretty safe. But in the same time, host_updated will be still general, so users would need to manually re-subscribe webhooks to a different event.

I agree that with the last change here, users can no longer receive events if a host was updated by facts upload, but at the same time, if there will be a request, we could add either a custom facts_upload event, or host_updated_facts or even again host_updated_automatically.

Other than that, I'd personally prefer the initial fix (without second commit), but I can be missing something, so I'd like to see what direction here we would all like to take, @adamruzicka, @ares.

@adamruzicka
Copy link
Contributor

we could add either a custom facts_upload event, or host_updated_facts

I had something like that in my mind. Spamming events is definitely wrong, but not firing anything when facts get uploaded doesn't feel right either

@ofedoren
Copy link
Member Author

@adamruzicka, I've added host_facts_updated event, but I guess it will work only on Host::Managed.

It seems we'd need to check how webhooks work for unmanaged and discovered hosts (are there any other?).

@ofedoren ofedoren force-pushed the refs-36104-explicit-host-update branch from edca111 to fc4e2d3 Compare August 10, 2023 15:26
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Works well, one inline comment

app/services/host_fact_importer.rb Outdated Show resolved Hide resolved
@ofedoren ofedoren force-pushed the refs-36104-explicit-host-update branch from fc4e2d3 to f421788 Compare August 21, 2023 11:39
@ofedoren
Copy link
Member Author

Thanks, @adamruzicka, updated. AFAIK it could throw NoMethodError, but apart from that I'm not sure it can raise something else, but just to be sure there is a catch for any exception.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Looks good to me, let's get this in

@adamruzicka
Copy link
Contributor

Katello failures seem unrelated

@ofedoren
Copy link
Member Author

Yeap: Katello/katello#10701

[test katello]

@adamruzicka
Copy link
Contributor

Could you please squash the commits before we merge?

@ofedoren ofedoren force-pushed the refs-36104-explicit-host-update branch from f421788 to 13698c3 Compare August 22, 2023 10:45
@ofedoren
Copy link
Member Author

Sure, done.

@adamruzicka
Copy link
Contributor

Test failures are unrelated and are being resolved in another PR. Let's get this in

@adamruzicka adamruzicka merged commit def66d5 into theforeman:develop Aug 22, 2023
4 checks passed
@adamruzicka
Copy link
Contributor

Thank you @ofedoren !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants