Skip to content
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

Update to Rails 7.1 #2822

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Update to Rails 7.1 #2822

wants to merge 18 commits into from

Conversation

alexdesi
Copy link
Collaborator

@alexdesi alexdesi commented Mar 7, 2025

What

  • Update rails 7 -> 7.1 (and related packages)
  • Refactor: remame Feature -> FeatureFlag
    for consistency and "zeitwerk" name convention
  • Refactor: move Faken into spec/support - since it is used just for testing
  • update JS package to rails/ujs@^7.1.3-4
  • Removed few warnings

Why

To keep VCD updated and secure

@alexdesi alexdesi force-pushed the ACD-588-update-rails-7.2 branch from eb3a5b7 to c2f801e Compare March 8, 2025 21:45
@alexdesi alexdesi changed the title Update to Rails 7.2 Update to Rails 7.1 Mar 10, 2025
&& bundle config --global without test development \
&& bundle config build.nokogiri --use-system-libraries \
&& bundle check || bundle install --jobs=4 --retry=3
# RUN gem install bundler -v $(cat Gemfile.lock | tail -1 | tr -d " ") \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

todo: Revert this as unique cmd

patrick-laa
patrick-laa previously approved these changes Mar 10, 2025
Copy link

@patrick-laa patrick-laa left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the dockerfile changes get reverted. (Alternatively, if we leave the RUN statements in and delete the commented out code I think that would also be fine, as that's easier to debug).

@alexdesi alexdesi marked this pull request as ready for review March 10, 2025 13:34
@alexdesi alexdesi requested a review from a team as a code owner March 10, 2025 13:34
@alexdesi alexdesi force-pushed the ACD-588-update-rails-7.2 branch from 30cc21b to dea60fd Compare March 10, 2025 17:18
@alexdesi alexdesi force-pushed the ACD-588-update-rails-7.2 branch from dea60fd to 2c77289 Compare March 10, 2025 17:20
@alexdesi alexdesi force-pushed the ACD-588-update-rails-7.2 branch 3 times, most recently from 3e2020c to 047de41 Compare March 11, 2025 12:58
@alexdesi alexdesi force-pushed the ACD-588-update-rails-7.2 branch from 047de41 to 19a321c Compare March 11, 2025 13:16
@alexdesi alexdesi force-pushed the ACD-588-update-rails-7.2 branch from 631de55 to ae568d1 Compare March 11, 2025 14:46
end

# see https://github.com/shadabahmed/logstasher
config.logstasher.enabled = true

Choose a reason for hiding this comment

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

Are we deliberately switching off logstasher in production?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no,
I think rails update removed it, and I forgot to put it back

Comment on lines -67 to -84
# Log disallowed deprecations.
config.active_support.disallowed_deprecation = :log

# Tell Active Support which deprecation messages to disallow.
config.active_support.disallowed_deprecation_warnings = []

# Use default logging formatter so that PID and timestamp are not suppressed.
config.log_formatter = Logger::Formatter.new

# Use a different logger for distributed setups.
# require "syslog/logger"
# config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name')

if ENV.fetch('RAILS_LOG_TO_STDOUT', nil).present?
logger = ActiveSupport::Logger.new($stdout)
logger.formatter = config.log_formatter
config.logger = ActiveSupport::TaggedLogging.new(logger)
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should revert these line back

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