Allow rails 7 gems in gemspec

[jrafanie]

Allow rails 7 gems in gemspec

Depends on:

• Allow rails 7 gems in gemspec activerecord-id_regions#33
• New id_regions release (0.4.0)
• Extracted backward compatible changes: Backward compatible rails 7 changes #735
• Fix failures creating associations in migration specs due to ActiveRecord::AssociationTypeMismatch errors. Rails 7 only. (associations classes not matching stub models)
• Fix "secrets" related test failures with rails 7 (mock/stubs on stub models)
• Cleanup ENV stubbing 20180606155924_move_ansible_container_secrets_into_database (workaround inability to mock/stub stub models).

The commits should be reviewable one by one.

Rails 7 changed the way migrations are run. The migration class
constant is now removed and the file is reloaded between migrations.

This means tests should not use expect/allow on migration stub classes
as the constant stubbed is not the same constant the migration is run against.

Additionally, comparisons of objects created before the migration with ones
after will also not work. In that case, you can assert by id or other
attributes instead of comparing objects.

Rails also checks association classes are correct and will also not match
expectations. For example, instead of using host to create or query
the host association, use host_id instead. This also works with has_manys
such as miq_product_feature_ids.

See rails/rails#42198

[miq-bot]

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

[kbrock]

rails 7.0 supports ruby 3.0 and 3.1 - so at least according to that, it should work

[jrafanie]

Oh, these are fun errors... looks like our migration_stub method is returning a cached constant and any associations you define in your migration "stub" fail under rails 7 because it's not a refresh association class.

[kbrock]

sigh. ping if you need to pair on this.
no ideas at this time

[jrafanie]

Ok, all green, now to clean up the ENV stubbing migration 20180606155924_move_ansible_container_secrets_into_database

[jrafanie]

We're still defaulting to rails 6.1 for new migrations.

[jrafanie]

These methods were added to inject different behavior in test vs production. This is because we can't inject this behavior into the migration stub classes. See a9ac761

[kbrock]

can we change the definition of CA_CERT_FILE instead?

CA_CERT_FILE = ENV[] || "const".freeze

and do these need to be test only?

[kbrock]

no. ignore

can we just do:

def ca_cert_file
  ENV.fetch("CA_CERT_FILE", "/run/secrets/kubernetes.io/serviceaccount/ca.crt")
end

or maybe ENV[].presence || ""

[jrafanie]

and do these need to be test only?

Yes, my objective was to not change the migration to add test code into it. I think what is here is a fair enough workaround to avoid test code in migrations while still being able to work with rails 7 neutering the ability to do allow/expect on the stub migration models.

[jrafanie]

def ca_cert_file
  ENV.fetch("CA_CERT_FILE", "/run/secrets/kubernetes.io/serviceaccount/ca.crt")
end

I was trying not to change the behavior of production migrations. While the proposed code reads, I really didn't want to change the behavior of past migrations.

[jrafanie]

Avoid the expectation on the migration stub constant since it's been removed and re-loaded by the migration.

[kbrock]

I also like avoiding expect all together.

👍

[jrafanie]

avoid calling the association methods because rails does a class lookup check which will fail with rails 7 since our stub class constant has been removed by rails 7 by the time this is run and so it doesn't match the expected association class object.

[jrafanie]

Again, avoid expectations on the migration class constant as it's been removed and re-loaded by the time these expectations are reached.

[jrafanie]

Ok, all green and cleaned up. The commits should be reviewable one by one.

[miq-bot]

Checked commits jrafanie/manageiq-schema@ae07b43~...9d49ecc with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
35 files checked, 0 offenses detected
Everything looks fine. ⭐

[Fryguy]

Can you explain why we need the id changes? There are a gazillion of them. Are the relationships lost or something in the cached stubs?

[jrafanie]

Can you explain why we need the id changes? There are a gazillion of them. Are the relationships lost or something in the cached stubs?

Sure, I had it in each commit. Good idea. I just added it to the description:

Rails 7 changed the way migrations are run. The migration class
constant is now removed and the file is reloaded between migrations.

This means tests should not use expect/allow on migration stub classes
as the constant stubbed is not the same constant the migration is run against.

Additionally, comparisons of objects created before the migration with ones
after will also not work. In that case, you can assert by id or other
attributes instead of comparing objects.

Rails also checks association classes are correct and will also not match
expectations. For example, instead of using host to create or query
the host association, use host_id instead. This also works with has_manys
such as miq_product_feature_ids.

See rails/rails#42198

[kbrock]

@jrafanie so you are converting from relations to ids so we can avoid defining relations all together? (e.g.: ext_management_system => ems to ems_id => ems.id)

Seems those are very non-rails specific and can be pulled out of here?

[jrafanie]

@jrafanie so you are converting from relations to ids so we can avoid defining relations all together? (e.g.: ext_management_system => ems to ems_id => ems.id)

Not specifically. I think that's beyond the scope here. The problem is that the association itself has logic to verify the class for the association. This verification doesn't work if you're trying to create an association with an object that is not the expected constant. This worked in 6.1 because the migration was loaded only once and the stub model constant remained. With rails 7, the migration stub model constants are removed and the migration is loaded again, so the classes aren't equivalent. What I'm doing here is avoiding the association method when I'm creating or comparing objects.

The migrations that define the associations can still use those association methods but I'm trying to avoid them in the tests for the issues found with rail 7.