Fixes #37825 - Upgrade Rails to 7.0 #10299
Conversation
| class ::FakePlugin; end | ||
| class ::FakePlugin::FakeModel; end | ||
| class ::FakePlugin::FakeModel < ApplicationRecord; end |
There was a problem hiding this comment.
Should this even be
module FakePlugin
class FakeModel < ApplicationRecord
end
end
ofedoren
force-pushed
the
rails-7.0
branch
2 times, most recently
from
b2971c8
to
c93acc6
Compare
|
There left only one test failure, I'm looking into it, for now I've got no clue :/ Apart from that, it's ready unless we want to be more correct and apply/incorporate these changes: https://railsdiff.org/6.1.7.8/7.0.8.4 |
|
Now it fails due to ldap_fluff, since it's locked on active_support < 7 (https://github.com/theforeman/ldap_fluff/blob/master/ldap_fluff.gemspec#L21). Weird that it didn't fail before... Also, @ekohl and @adamruzicka or maybe @ShimShtein, since you have more experience with the codebase and Rails, could you please help me get why the test fails? I'll be out for the next week (30.09 - 06.10), so I hope we won't waste that week. Regarding the test: I think it's due to code reloading happening somewhere in the |
|
Finally, it's green and with minimal changes needed. Ready for full review. |
|
As noted in a meeting, the failure may be due to nulldb: foreman/.github/workflows/foreman.yml Line 73 in 17f6c03 |
There was a problem hiding this comment.
@ofedoren I see you now disable spring for integration tests and enable bootsnap for tests. Do you consider the issue resolved with that?
Additionally, is there a way we could use spring for integration tests? Possibly by explicitly stopping the server `
| @@ -10,7 +10,8 @@ | |||
| # test suite. You never need to work with it otherwise. Remember that | |||
| # your test database is "scratch space" for the test suite and is wiped | |||
| # and recreated between test runs. Don't rely on the data there! | |||
| config.cache_classes = true | |||
| # Disable reloading for integration tests | |||
| config.cache_classes = ARGV.grep(/test\/integration/).any? | |||
There was a problem hiding this comment.
Shouldn't this be the other way around? Only enable it when it's none??
Also, https://github.com/rails/spring?tab=readme-ov-file#enable-reloading notes it needs to be false for spring to work.
Another note: https://github.com/rails/spring?tab=readme-ov-file#temporarily-disabling-spring says:
If you're using Spring binstubs, but temporarily don't want commands to run through Spring, set the DISABLE_SPRING environment variable.
I'd say so, yes. Mostly because of spring being disabled for integrations. I don't think bootsnap did anything though. I just enabled that since it should be quite safe and maybe help us in the future.
How that will help except for running integration tests on local dev setup? I mean, the whole point of spring for tests is to reduce time between individual calls/runs after the first one. In CI it doesn't matter, since there is only one (the first) run. For local dev runs it makes life easier, thus I've tried to leave the Honestly, I'm quite surprised that reloading for units/functionals seems to work at all. I'd say reloading was (and still is) mostly unsupported within Foreman and even if it worked, it kidna shouldn't (I might be wrong though, just an impression).
Yes,
Yeap, noticed that too and I thought to use that for CI workflow, but then it would make devs use it for their runs, which I've tried to avoid. This PR (the commit with spring conditionally disabled) tries to make as little invasive changes as possible without changing any already existing workflows: spring is not nuked, it can be still used by devs (although it won't work properly anyway), it's even supported for most test suits (except integrations), no additional env variables must be set to run commands. I'd love having code reloading working properly, but it seems it's quite complicated to make it work in Foreman (I might lack deeper knowledge about Rails internals though, so if anyone wants to fix that, please do. Fresh eyes make miracles). I disabled spring for integration tests since reloading takes too much time (although it shouldn't) causing timeouts, messes up constant ids, which leads to errors such as |
ofedoren
force-pushed
the
rails-7.0
branch
2 times, most recently
from
2f040d6
to
1a151be
Compare
Without this patch Ansible-related scopes for Host::Managed.includes()
would look like:
[ { host_ansible_roles: :ansible_roles }, { ansible_role: [{ ansible_variables:
[:lookup_values] }] } ], which for some reason worked before Rails 7.
After this patch, it looks like:
[ { host_ansible_roles: { ansible_role: [{ ansible_variables:
[:lookup_values] }] } } ].
There was a problem hiding this comment.
Can we clear https://github.com/theforeman/foreman/blob/develop/config/as_deprecation_whitelist.yaml? I think those deprecations should now be resolved and will make CI fail on them. Plugins should also respect this config.
Sure, done. |
Based on #10131, but with additional commit needed for Rails upgrade.