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

Actualizar Rails a 7.2.0 #515

Merged
merged 7 commits into from
Aug 19, 2024
Merged

Actualizar Rails a 7.2.0 #515

merged 7 commits into from
Aug 19, 2024

Conversation

santiagorodriguez96
Copy link
Collaborator

No description provided.

Starting in Rails 7.2, pid files are not used anymore in production:

rails/rails#50644
We are experiencing some issues to deploy our changes, as the Docker
build step is failing with the following error:

```
 > [build 3/6] RUN bundle install &&     rm -rf ~/.bundle/ "/usr/local/bundle"/ruby/*/cache "/usr/local/bundle"/ruby/*/bundler/gems/*/.git &&     bundle exec bootsnap precompile --gemfile:
14.33 Installing warden 1.2.9
14.34 Fetching rack-protection 4.0.0
14.35 Installing rack-protection 4.0.0
14.36 Fetching sprockets 4.2.0
14.38 Installing sprockets 4.2.0
14.41 Fetching websocket-driver 0.7.6
14.42 Installing websocket-driver 0.7.6 with native extensions
21.46 Downloading net-pop-0.1.2 revealed dependencies not in the API or the lockfile
21.46 (net-protocol (>= 0)).
21.46 Running `bundle update net-pop` should fix the problem.
------
Dockerfile:35
--------------------
  34 |     COPY Gemfile Gemfile.lock .ruby-version ./
  35 | >>> RUN bundle install && \
  36 | >>>     rm -rf ~/.bundle/ "${BUNDLE_PATH}"/ruby/*/cache "${BUNDLE_PATH}"/ruby/*/bundler/gems/*/.git && \
  37 | >>>     bundle exec bootsnap precompile --gemfile
  38 |
--------------------
ERROR: failed to solve: process "/bin/sh -c bundle install &&     rm -rf ~/.bundle/ \"${BUNDLE_PATH}\"/ruby/*/cache \"${BUNDLE_PATH}\"/ruby/*/bundler/gems/*/.git &&     bundle exec bootsnap precompile --gemfile" did not complete successfully: exit code: 34
```

After digging around for a bit it seems that the temporary solution
until we update to Ruby 3.3.4 is to add it from the ruby repo as a
dependency.
Gemfile Outdated
Comment on lines 11 to 12
# Needed until we update Ruby to 3.3.4 https://discuss.rubyonrails.org/t/rails-7-dockerfile-build-fails/86041/2
gem 'net-pop', github: 'ruby/net-pop'
Copy link
Collaborator

Choose a reason for hiding this comment

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

si no hay nada que nos bloquee usar 3.3.4 ya, yo propondria primero mergear el PR de 3.3.4 asi no tenemos que hacer esto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Estoy de acuerdo!

Copy link
Collaborator Author

@santiagorodriguez96 santiagorodriguez96 Aug 14, 2024

Choose a reason for hiding this comment

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

Listo: c452dd7

This reverts commit e5c528b – now that
we've updated Ruby to 3.3.4
…ency

For some reason we need to run `bundle update net-pop` in order for the
`Gemfile.lock` to include `net-pop`'s dependency on `net-protocol`, so
that we ensure the latter is installed before the former and we don't
run into the deploy issue that we were talking about in previous
commits.
@EmilioCristalli
Copy link
Collaborator

Algunas diferencias que veo con una app nueva de Rails 7.2:

  1. .github/workflows/ci.yml: agregan varias cosas que puede estar bueno usar (no se si ahora en este PR o en el futuro) como bin/brakeman --no-pager y importmap audit
  2. app/controllers/application_controller.rb: opiniones sobre agregar allow_browser versions: :modern?
  3. bin/brakeman: seria bueno agregar esto cuando/si empezamos a usar brakeman
  4. bin/bundle : hay algunas diferencias q seguramente no cambien el comportamiento, pero capaz vale la pena dejarlo como en 7.2?
  5. config/initializers/new_framework_defaults_7_*.rb : todavia tenemos los de 7.1 para habilitar. Deberiamos crear un ticket para habilitar los de 7.1 y 7.2 desp de mergear este PR
  6. config/puma.rb el nuevo default de threads_count es 3, deberiamos cambiarlo?
  7. public/406-unsupported-browser.html : si agregamos el allow_browser lo vamos a precisar. Y capaz vale la pena tenerlo de todas formas?

@santiagorodriguez96
Copy link
Collaborator Author

  1. .github/workflows/ci.yml: agregan varias cosas que puede estar bueno usar (no se si ahora en este PR o en el futuro) como bin/brakeman --no-pager y importmap audit

Yo estaba pensando en agregarlos como parte de próximos PRs. Voy a crear unos tickets para que quede trackeado eso.

  1. app/controllers/application_controller.rb: opiniones sobre agregar allow_browser versions: :modern?

Para mi tiene sentido! Qué te parece dejarlo para un próximo PR?

  1. bin/brakeman: seria bueno agregar esto cuando/si empezamos a usar brakeman

Estoy de acuerdo 👍

  1. bin/bundle : hay algunas diferencias q seguramente no cambien el comportamiento, pero capaz vale la pena dejarlo como en 7.2?

Qué cambios te aparecen en bin/bundle? A mi no me aparece ninguno si tiro rails app:update. De todas formas ese archivo es generado por Bundler, por lo que no sé si hay una versión de bin/bundler asociada a Rails 7.2.

Será un tema de que actualizamos Bundler pero no volvimos a generar el archivo?

Si corro bundle binstubs bundler --force ahí si me cambia y me queda:

diff --git a/bin/bundle b/bin/bundle
index a71368e..50da5fd 100755
--- a/bin/bundle
+++ b/bin/bundle
@@ -27,7 +27,7 @@ m = Module.new do
     bundler_version = nil
     update_index = nil
     ARGV.each_with_index do |a, i|
-      if update_index && update_index.succ == i && a =~ Gem::Version::ANCHORED_VERSION_PATTERN
+      if update_index && update_index.succ == i && a.match?(Gem::Version::ANCHORED_VERSION_PATTERN)
         bundler_version = a
       end
       next unless a =~ /\A--bundler(?:[= ](#{Gem::Version::VERSION_PATTERN}))?\z/
@@ -41,13 +41,13 @@ m = Module.new do
     gemfile = ENV["BUNDLE_GEMFILE"]
     return gemfile if gemfile && !gemfile.empty?
 
-    File.expand_path("../../Gemfile", __FILE__)
+    File.expand_path("../Gemfile", __dir__)
   end
 
   def lockfile
     lockfile =
       case File.basename(gemfile)
-      when "gems.rb" then gemfile.sub(/\.rb$/, gemfile)
+      when "gems.rb" then gemfile.sub(/\.rb$/, ".locked")
       else "#{gemfile}.lock"
       end
     File.expand_path(lockfile)
@@ -60,24 +60,19 @@ m = Module.new do
     Regexp.last_match(1)
   end
 
-  def bundler_version
-    @bundler_version ||=
-      env_var_version || cli_arg_version ||
-        lockfile_version
-  end
-
   def bundler_requirement
-    return "#{Gem::Requirement.default}.a" unless bundler_version
-
-    bundler_gem_version = Gem::Version.new(bundler_version)
-
-    requirement = bundler_gem_version.approximate_recommendation
+    @bundler_requirement ||=
+      env_var_version ||
+      cli_arg_version ||
+      bundler_requirement_for(lockfile_version)
+  end
 
-    return requirement unless Gem::Version.new(Gem::VERSION) < Gem::Version.new("2.7.0")
+  def bundler_requirement_for(version)
+    return "#{Gem::Requirement.default}.a" unless version
 
-    requirement += ".a" if bundler_gem_version.prerelease?
+    bundler_gem_version = Gem::Version.new(version)
 
-    requirement
+    bundler_gem_version.approximate_recommendation
   end
 
   def load_bundler!

Podemos actualizarlo en otro PR si te parece 🙂

  1. config/initializers/new_framework_defaults_7_*.rb : todavia tenemos los de 7.1 para habilitar. Deberiamos crear un ticket para habilitar los de 7.1 y 7.2 desp de mergear este PR

Tiene sentido!

  1. config/puma.rb el nuevo default de threads_count es 3, deberiamos cambiarlo?

Yo por ahora elegí no cambiarlo, pero si les parece que tiene sentido lo cambiamos, si.

  1. public/406-unsupported-browser.html : si agregamos el allow_browser lo vamos a precisar. Y capaz vale la pena tenerlo de todas formas?

Yo creo que sería bueno agregarlo cuando agreguemos el allow_browser. Más que nada por un tema de completitud de la historia de git (o sea que puedamos encontrar todos los cambios para añadir el allow_browser en el mismo PR/commit).

Copy link
Collaborator

@EmilioCristalli EmilioCristalli left a comment

Choose a reason for hiding this comment

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

por mi ok con hacer los cambios después (creemos un ticket para no olvidarnos)

Los cambios que veo del bin/bundle son los que pusiste ahí

@santiagorodriguez96
Copy link
Collaborator Author

Dale! Ya creé los tickets relacionados a lo que hablamos 👍

@santiagorodriguez96 santiagorodriguez96 merged commit 671af70 into master Aug 19, 2024
1 check passed
@santiagorodriguez96 santiagorodriguez96 deleted the sr--bump-rails-to-72 branch August 19, 2024 13:23
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