Add safe navigation if ip is nil on hosts sync

theforeman / foreman_rh_cloud

a plugin to Foreman that generates and uploads reports to the Red Hat cloud

GNU General Public License v3.0

6 stars 30 forks source link

Add safe navigation if ip is nil on hosts sync #885

Closed chris1984 closed 3 months ago

chris1984 commented 3 months ago

What are the changes introduced in this pull request?

Adding a skip if ip_addresses is nil

What are the testing steps for this pull request?

Register a host, run inventory sync
Unregister the host to run the missing hosts method
Add in ip_addresses = nil at https://github.com/theforeman/foreman_rh_cloud/blob/v10/lib/inventory_sync/async/inventory_hosts_sync.rb#L70
Run foreman-rake rh_cloud_inventory:sync and watch the production.log fail with undefined method take_while for nil:NilClass (NoMethodError)
Apply PR and restart services
Try the task and see that it completes without a traceback

chris1984 commented 3 months ago

@ShimShtein let me know what i'm missing with the test, it's still asserting 2 instead of 3. or we can talk about it tomorrow when we sync up.

ShimShtein commented 3 months ago

@chris1984 you are not missing anything, actually you are adding too much here: https://github.com/theforeman/foreman_rh_cloud/pull/885/files#diff-1db2fc1b0779ffd29ad0788780a64b71490bad277d33887ef72e1283465b09d5R42

If you create a record in foreman, and create it in the Insights request mock, it means that it's not a missing host. This is why you have only two missing hosts, even though you have added a new one to the Insights request. If you want to test for creation of a new missing host record, you need to make sure it exists only in Insights, and not in Foreman.

chris1984 commented 3 months ago

@ShimShtein updated, thanks for the pointer

ShimShtein commented 3 months ago

Merged, thanks @chris1984 !