Fix rubocop offenses

[archanaserver]

No description provided.

[archanaserver]

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.

[stejskalleos]

Tests are failing but I guess not because of the changes here. I need to take a look what's going on there.

[ekohl]

I think the benefit isn't really there with that cop. Looks just like a style thing to me so I'd initially ignore it. Haven't looked at the reversible cop yet

[ekohl]

We can see in #615 that the test suite is already broken. That makes it hard to judge if these items break anything (further) or not.

[stejskalleos]

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.

[ekohl]

In ruby tests, there is one test that is failing everywhere:

This is:

foreman_discovery/test/unit/host_discovered_test.rb

Lines 263 to 277 in 6aaa8d2

	test "should refresh facts and NICs of an existing discovered host" do
	host1 = discover_host_from_facts(@facts)
	assert_equal 'mace41f13cc3658', host1.name
	assert_equal 'IBM System x -[7870K4G]-', host1.facts["productname"]
	assert_equal 1, Host::Discovered.where(:name => 'mace41f13cc3658').count
	assert_equal '10.35.27.2', host1.ip
	@facts["ipaddress_eth0"] = "1.2.3.4"
	@facts["productname"] = "Dishwasher DW400"
	host2 = discover_host_from_facts(@facts)
	assert_equal 'mace41f13cc3658', host2.name
	assert_equal 'Dishwasher DW400', host2.facts["productname"]
	assert_equal '10.35.27.2', host2.ip
	assert_equal 1, Host::Discovered.where(:name => 'mace41f13cc3658').count
	end

It's updating the legacy fact productname while since theforeman/foreman@cc4a95f 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.

[ekohl]

It could also be caching facts via theforeman/foreman@735b401.

[ekohl]

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 #617 fixes that particular test.

[ekohl]

#618 fixes another test that has long been broken.

[stejskalleos]

I merged fix for broken rake foreman_discovery:test , can you rebase and squash the commits?

[ekohl]

@stejskalleos I wouldn't squash commits because it's easier to review the changes per cop

[stejskalleos]

@archanaserver can you rebase please so the CI will get run on the latest changes?
(You don't have to squash the commits, as Ewoud said)

[archanaserver]

@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?

[archanaserver]

@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 theforeman/foreman_default_hostgroup#62 also has similar kind of failure https://github.com/theforeman/foreman_default_hostgroup/actions/runs/8135910181/job/22231270234?pr=62

[ekohl]

As you noted, we see the failures in other places too. It's being worked on. It doesn't have to block merging.

[ekohl]

You can drop this since #618 removed the assertion and created a better one.

[archanaserver]

@ekohl @stejskalleos would you mind taking a look on the final changes?

[ekohl]

@stejskalleos?

[stejskalleos]

Thanks @archanaserver @ekohl