Skip to content

Fix undefined Rails version for other repos sub-builds #2569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

pirj
Copy link
Member

@pirj pirj commented Jan 26, 2022

@pirj pirj self-assigned this Jan 26, 2022
@@ -32,7 +32,7 @@ end
# automatically included. This issue was fixed on the Rails
# side in Rails 7.0.1, but is not yet fixed in the Rails 6.1.x
# branch. Discussion can be found here - https://github.com/mikel/mail/pull/1439
if RUBY_VERSION >= '3.1' && version.split(' ').last < '7.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just do version = ... at the top and run the case statement of a local variable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We skip this for:

  • pre 6.1 because it doesn't have Action Mailbox
  • 7.0.1+ because it does the same (presumably) itself

The only -stable build left is 5-2-stable that we'll drop.

In any case, this doesn't hurt on 3.1. And there's no other way to work around this for 6.1.

I don't really want to overcomplicate this.

Ideally, we could:

- version = ENV['RAILS_VERSION'] || (File.exist?(version_file) && File.read(version_file).chomp) || ''
+ version = ENV['RAILS_VERSION'] || (File.exist?(version_file) && File.read(version_file).chomp) || "~> 6.0.0"

and combine the last two branches.

This would allow to keep && version.split(' ').last < '7.0'.
Is it worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could return from the case statement a skip variable? Seems like skipping it on rails 7 is a good idea to ensure we don't change its behaviour?

@pirj
Copy link
Member Author

pirj commented Jan 27, 2022

I've just realized that it's not only for Rails 6.1.
On Rails 6.0 that is used in sub-builds, Action Mailer loads the mail gem, and it:

require 'net/smtp'

Adjusting accordingly.

@pirj pirj force-pushed the fix-sub-builds-2 branch from 4ab1ed7 to 383bd23 Compare January 27, 2022 07:48
@JonRowe JonRowe merged commit 58fee01 into main Jan 27, 2022
@JonRowe JonRowe deleted the fix-sub-builds-2 branch January 27, 2022 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants