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

Rails7 upgrade #689

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Rails7 upgrade #689

merged 6 commits into from
Dec 21, 2023

Conversation

ice1080
Copy link
Contributor

@ice1080 ice1080 commented Dec 9, 2023

Context

Summary of Changes

  • upgraded to 7.0.8
  • upgraded to 7.1.2

Checklist

  • Tested Mobile Responsiveness
  • Added Unit Tests
  • CI Passes
  • Deploys to Heroku on test Correctly (Maintainers will handle)
  • Added Documentation (Service and Code when required)

@ice1080
Copy link
Contributor Author

ice1080 commented Dec 9, 2023

@DeeDeeG once #688 gets merged I can merge or rebase.
I also see the same single unit test failure on this, but it works manually.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 11, 2023

Thanks for this!

I've read the diff once over, and it all generally looks good (and a mostly small functional diff, considering a major version upgrade of our main framework), considering the tests are passing in CI. (Other than the one known issue from before this PR as you mentioned).

I hope to get a chance to manually test it myself soon, take a look around at the finer details, just look through more thoroughly.

But this will be great to have, with this we'll pretty much have all our fundamental dependencies like Node, Ruby and Rails up to date! (JS and Ruby dependencies not doing too badly in some senses either.) [EDIT: Efforts to move away from Rails' Webpacker would still be an improvement as well, of course! And any possibility of a JS full app re-write, etc. But keeping the current stuff up to date is important!]

Once again, thanks, and I'll hope to review this one properly soon! (Open to input from other maintainers, but this seems like a small enough change and with CI looking good that I'm hopeful this is a fairly easy upgrade.)

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 15, 2023

Hi again, just checking in / providing a status update:

I have another project that I have been busy with for the past few days (that project gets especially busy leading up to/around the 15th.)

I hope to make some time for this after the other project settles down for the month. Ideally I'd like to get my personal Heroku account set up for testing, if the other maintainers haven't set up testing environments on the main account before then. Especially for the NodeJS bump. I may merge this one first if this one proves easier to test locally.

Best Regards.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 16, 2023

I have the chance to test this manually now.

Local/manual testing is working out very well for basically the whole app, barring one part.

The API docs page /api/docs (like https://refugerestrooms.org/api/docs/) is not loading its content. So, I suppose something about how JavaScript is loaded may have changed?


We have some files related to loading these API docs:

  • app/javascript/packs/application.js
  • app/javascript/packs/views/api/docs/index.js
  • app/views/api/docs/index.html
  • app/controllers/api/v1/base.rb
  • config/routes.rb

-- One can see the PR where it was implemented in this form here: #506


If it is possible to get this working again, that would be very good. No specs test the docs page working at the moment, so it is tested by manual testing only for now.

Best Regards.

@ice1080
Copy link
Contributor Author

ice1080 commented Dec 20, 2023

Ah interesting @DeeDeeG , I'll look into this. I definitely tested them manually at one point but something else I did must have broken them. Will get a fix out!

@ice1080
Copy link
Contributor Author

ice1080 commented Dec 20, 2023

Hmm, I see /api/docs working when I run it locally using docker-compose up. If you run this branch locally are you able to see it working? Looking into switching to the prod config locally if possible now to see if I can reproduce

@ice1080
Copy link
Contributor Author

ice1080 commented Dec 20, 2023

@DeeDeeG I wasn't able to reproduce locally since it seems to be a production only issue and I don't have the necessary config vars to run prod locally. I pushed one change blind that seems like it could be related, from the rails docs, but I can't test it so need you to try that one more time when you get some time

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 21, 2023

My apologies, it appears the issue was just on my Docker setup. I deleted all volumes and images built for Refuge and rebuilt them (docker compose build and docker compose up) and then it worked.

Extra details about the error I was seeing (now resolved) (click to expand if curious):

For what it's worth, the error was something to do with Webpacker not finding some function it wanted to call. There was some sort of bundle JS file it was calling into, and I'm not sure what was wrong there exactly, but as mentioned it did go away after I rebuilt the image, regardless of with/without the last commit in this branch.

Uncaught TypeError: __webpack_require__(...).start is not a function
    js application.js:2
    Webpack 3
        __webpack_require__
        <anonymous>
        <anonymous>

The latest commit did not seem to make the difference, so given it seems unrelated to the issue, I'd like to ask that it be reverted, or I can certainly do that if you prefer, and I would be ready to merge this PR at that point.

Thank you very much once again!

@ice1080
Copy link
Contributor Author

ice1080 commented Dec 21, 2023

So glad to hear it worked! Reverting the commit now

@ice1080
Copy link
Contributor Author

ice1080 commented Dec 21, 2023

@DeeDeeG just reverted the latest commit (f465bd46c1d900cd9e642954fd07cd384b0289f0). Should be ready now!

Please note that there are a few configs that are a bit more advanced that should be reviewed as well. These configs live in the following files:

  • new_framework_defaults_7_0.rb
  • new_framework_defaults_7_1.rb

For longer term maintenance, someone with more rails experience should review those and work on getting the project updated with those files using these guides:

I noticed in application.rb that we're still using defaults from rails 5 so each future rails version will get harder to update.

Anyway though, this rails7 upgrade should help with some of the other issues in the repo and we can tackle those later. Thanks!

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 21, 2023

just reverted the latest commit

Thank you!

a few configs that are a bit more advanced that should be reviewed as well [ . . . ] new_framework_defaults*.rb

Yes, I suppose we need to go through the backlog of all these, flip them one by one, test one by one, and so on. Should be doable. I think we've been reluctant to put a chunk of time into verifying something that might maybe break the app in an obscure way if we don't catch it, but it'd be good to enable defaults and drop these configs that effectively put us at some risk of having the old behavior deprecated/removed for us by a newer Rails version that comes out... I shall have it in the back of my mind on my to-do list, at minimum. Maybe I can carve out time to work through them all (or a good chunk of them every so often) some time soon... In any case, progress is progress!

I'm confident in merging this one. A big thanks for this upgrade PR.

And then, since #688 needs some testing with a Heroku account to confirm the config it pushes to Heroku is adequate to have the app build successfully, I'll work that out on a test dyno on a real Heroku account. If that all goes well, I can push all of these updates out to production together. If the Node update hits a snag, I'll probably push the rest out first and troubleshoot that one if it takes a lot of doing.

Anyway, future plans aside, this one is good to go! Much appreciated.

@DeeDeeG DeeDeeG merged commit aa22bf7 into RefugeRestrooms:develop Dec 21, 2023
1 check failed
@mmx900 mmx900 mentioned this pull request May 5, 2024
5 tasks
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.

Upgrade Rails to v7
2 participants