Conversation
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
There are some things to review and fix (see inline comments), but overall this looks good.
I have a few general/overview questions as well:
-
Was
bundle exec rails app:updaterun? The lack of changes inconfig/surprises me.Unfortunately, that is usually the most time-consuming part of updating Rails apps when I was doing it. The format/style doesn't match ours at all, so you need to use
d(diff) and apply the relevant changes manually. -
For the memory issues seen in CI, I'm wondering if adding this to
config/application.rbwould fix it:config.yjit = false
If it does, I would hazard that we could be more specific; perhaps putting it behind
if ENV['CI']or inconfig/environments/test.rb. Still worth a try to see if it fixes it, though. -
Do we want to mix the Twitter removal ticket with the dependency updates?
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
Glad to see rails app:update wasn't quite the heavy diff I feared it could be. There's a few things we should discuss before merging.
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
in general, i think this looks good, but i'm a little concerned about the changes to the build and i'm not sure how far we've gone into investigating the cause.
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
We're getting there! Rails upgrades can be a pain, and I really appreciate all the work you've been doing.
There's a few minor nits that I have found (around formatting and comments), and then the existing review feedback that maría and I have left that isn't marked resolved.
This is still shaping up very nicely, I'd say.
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
Left inline comments on converting the SECRET_KEY_BASE to a Docker secret so that can be tested properly. Otherwise, this is really looking good!
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
After the params.expect question is resolved, this is great. Great job getting this all updated! r+
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
r+, agreed with @awilfox. looks like there's a merge conflict to resolve too. once those two items are done this is good to go.
|
What's the decision on params.require vs. params.expect? That was triggered by rubocop. It appears that the way it is now (with params.expect) it was still allow empty values but checks that the structure is as expected. I can disable the rubocop check but it looks like params.expect is the better option to me in this case unless I'm missing something. https://medium.com/jungletronics/rails-8-understanding-params-expect-19c4585d1a1c |
|
It looks like you're right; |
dependency updates Pagy uses pagy.limit now for number of items on page updating depencies, moving to rails 8.0.4, ruby 3.4 and pagy 43 increased memory for tests increased memomory for app for tests, only run tests if setup succeeds changed based url for IIF server, removed memory allocation for compose.ci using selenium stand alone chrome, broke out steps in build.yml for precompile and db:prepare bringin up selenium after assets are precompiled, lowering the reserved memory for selenium bypassing yarn build -watch in ci since assets are precompiled in a different step only start selenium before rspec needs it bringing up compose without dependencies so sellenium doesn't start up early running precompile as a standalone. hope this fixes memory issue when building during test using dummy key_base for precompile for rspec need to have iipsrv, app and selenium running for rspec need to have iipsrv, app and selenium running need bundle exec when calling rspec and rubocop full rails server not needed for tests, using slimmed down version for tests running system tests as stand alone adding sleep infinity to docker-compose.ci, hopefully fixes memory issue running tests refactored do_profile in application_controller Added omniauth-rails_csrf_protection gem and updated omniauth intializer for post only removed commented out code, set config.yjit false and removed sleep infity from compose.ci ran rails app:update and made necessary changes to accommodate the update to rails 8 added sleep infinity back in to compose.ci, tests failing in build without it changed unauthorized user redirect so it goes to a page instead of directly to calnet removed some Gem dependencies, changed a comment in puma config, added static page instead of direct redirect to CAS fixed a fex minor styling issues in specs removed autoload of lib/assets as we dont' use that directory set rubocop and rspec to always run, removed commented sections out of docker-compose, remove unused logger setting for production, removed redundant to_s in borrower_toker.rb reverting build.yml to 1.7.9 release version, removed sleep infinity from compose.ci Added dummy secret for build.yml starting rails server in compose.ci so it doesn't run yarn build removed an outdated comments, some formatting changes in specs, put back in the build steps to handle memory issues added newline to end of spec fixed rubocopy formatting error using 3.3 instead of 3.4, changes for chrome driver settings, spec Added --add-platform x86_64-linux to Gemfile.lock Accounting for secret key base in build having docker image generate rails secret for SECRET_KEY_BASE Update .github/workflows/build.yml Co-authored-by: Anna Wilcox <AWilcox@Wilcox-Tech.com> Update docker-compose.ci.yml Co-authored-by: Anna Wilcox <AWilcox@Wilcox-Tech.com> Update docker-compose.ci.yml Co-authored-by: Anna Wilcox <AWilcox@Wilcox-Tech.com>
davezuckerman
force-pushed
the
AP-587-update-dependencies
branch
from
2defcb2 to
80b44aa
Compare
3 participants
I ran into issues with the build. It was running out of memory during the tests so I had to rework the test section of build.yml. I'm not sure why this is using so much memory though. Any thoughts? This currently builds but it won't using the previous build.yml