foreman_discovery
foreman_discovery copied to clipboard
theforeman
Fix rubocop offenses
I can see one rubocop offense on this cop Style/SlicingWithRange which we might have discussed in the past, https://docs.rubocop.org/rubocop/cops_style.html#styleslicingwithrange. Also this is unsafe, but what's your thought on this @ekohl, do we want this here?
And another one is Rails/ReversibleMigration.
Tests are failing but I guess not because of the changes here. I need to take a look what's going on there.
We can see in https://github.com/theforeman/foreman_discovery/pull/615 that the test suite is already broken. That makes it hard to judge if these items break anything (further) or not.
In ruby tests, there is one test that is failing everywhere:
Failure:
HostDiscoveredTest#test_0024_should refresh facts and NICs of an existing discovered host [foreman_discovery/test/unit/host_discovered_test.rb:274]
Minitest::Assertion: Expected: "Dishwasher DW400"
Actual: "IBM System x -[78[70](https://github.com/theforeman/foreman_discovery/actions/runs/7602436753/job/20703002127?pr=616#step:13:71)K4G]-"
I will take a look during this week.
The foreman_discovery was failing since I am a maintainer of the repo, I tried to reproduce the problem without any success.
In ruby tests, there is one test that is failing everywhere:
This is:
https://github.com/theforeman/foreman_discovery/blob/6aaa8d26a76c326f38cc87d9d40b331b1c381c57/test/unit/host_discovered_test.rb#L263-L277
It's updating the legacy fact productname while since https://github.com/theforeman/foreman/commit/cc4a95ffbe24ddc1bc16dd3f13ceef2a742331f9 we prefect the modern fact dmi.product.name. Though the fixture (https://github.com/theforeman/foreman_discovery/blob/develop/test/facts/regular_host.json) doesn't contain the modern fact at all.
It could also be caching facts via https://github.com/theforeman/foreman/commit/735b40161176be270934998a126b42e90c0e9724.
It could also be caching facts via theforeman/foreman@735b401.
From some testing it does appear to be failing it, which I could reproduce locally. I think https://github.com/theforeman/foreman_discovery/pull/617 fixes that particular test.
https://github.com/theforeman/foreman_discovery/pull/618 fixes another test that has long been broken.
I merged fix for broken rake foreman_discovery:test , can you rebase and squash the commits?
@stejskalleos I wouldn't squash commits because it's easier to review the changes per cop
@stejskalleos it is rebased and running on the latest changes.
The checks are failing due to some webpack changes, I have looked into it, this seems like some routing issue and I'm not sure what is the exact issue here. Would you mind taking a look here?
@ekohl i can't find why it is failing clearly, would you please help me here?
Edit: Also I guess foreman_default_hostgroup this repo https://github.com/theforeman/foreman_default_hostgroup/pull/62 also has similar kind of failure https://github.com/theforeman/foreman_default_hostgroup/actions/runs/8135910181/job/22231270234?pr=62
@ekohl @stejskalleos would you mind taking a look on the final changes?