Expose multi-CV feature downstream

[jeremylenz]

This removes allow_multiple_content_views from UpstreamOnlySettings, allowing it to be exposed downstream.

I didn't remove the UpstreamOnlySettings feature entirely, because I thought it might be handy to have in the future? thoughts welcome.

[evgeni]

Stubbing constants is weird (in Ruby?). You'd need stub_const from either rspec-mocks or minitest-stub-const for that, I think.

[jeremylenz]

Originally I was just setting them in the test, but that felt weird too. What's the non-weird way?

[ShimShtein]

I think it would be better to redesign the UpstreamOnlySettings module to be a proper class:

class UpstreamOnlySettings
  def self.include?(key)
    new.include?
  def include?(key)
    SETTINGS.include?(key.to_s)
  end
end

Then it would be easier to stub the class:

UpstreamOnlySettings.any_instance.stubs(:include?).returns(false)

[jeremylenz]

@ShimShtein good idea! updated.

I'm relying on CI to see if my tests are good, looks like someone needs to click a button for me

[jeremylenz]

@ShimShtein how's it looking now?

[jeremylenz]

guess I was trying to be a Pythonista...

updated

[ShimShtein]

LGTM. Let's wait for the tests

[jeremylenz]

I can't run tests locally, I am getting errors like

ERROR:  relation "permissions" does not exist (PG::UndefinedTable)
LINE 8:  WHERE a.attrelid = '"permissions"'::regclass

I'm guessing it's Zeitwerk-related somehow?...

[jeremylenz]

https://paste.centos.org/view/6a2ad24e

[ShimShtein]

@jeremylenz Currently I see only unmet expectations failures in the report: https://github.com/RedHatSatellite/foreman_theme_satellite/actions/runs/11223216416/job/31283723243?pr=85#step:21:36

Do you see something else?

[jeremylenz]

that's what I see too.

[jeremylenz]

I was finally able to update my dev box and see why the tests were failing. Should be passing now.

[jeremylenz]

🍏

[evgeni]

You never add test_setting to UpstreamOnlySettings::SETTINGS, so this test is not testing what it says to be testing.
Wouldn't you need to first declare that setting with a default value, then add it to UpstreamOnlySettings and then assert the result is nil?

[jeremylenz]

I can't

UpstreamOnlySettings::SETTINGS << 'test_setting'

due to

FrozenError: can't modify frozen Array: []

But I did add the setting definition in the test.
(and did confirm it does fail when I comment out the

UpstreamOnlySettings.expects(:include?).with('test_setting').returns(true)

)

does that look okay?

[jeremylenz]

Side note: I went thru some confusing test failures because Minitest is retrying the test. The retries aren't the actual failure:

[MinitestRetry] retry 'test_0002_hides upstream-only settings' count: 1,  msg: FrozenError: can't modify frozen Array: []
    /home/vagrant/foreman_theme_satellite/test/unit/setting_registry_branding_test.rb:42:in `block (2 levels) in <class:SettingRegistryBrandingTest>'
[MinitestRetry] retry 'test_0002_hides upstream-only settings' count: 2,  msg: Foreman::Exception: ERF42-8739 [Foreman::Exception]: Setting 'test_setting' is already defined, please avoid collisions

I can't find where the retries are configured 🤔

[jeremylenz]

@evgeni how's it look now?

[ShimShtein]

You are not defining the test_setting in settings here. Are you relying on the test execution order?

Also not related to this specific PR: why do we have two classes in a single file? Can we separate them please?

[evgeni]

I think this relies on the fact that Setting[something] always queries the Settings registry and triggers the warning code, even if the setting is not actually defined. (The test worked also before the setting definition in the previous test that I asked for)

[ShimShtein]

Merged. Thanks @jeremylenz !