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

ruby 2.7.2 and rails 7 upgrade #979

Merged
merged 18 commits into from
Jun 9, 2023
Merged

Conversation

yksflip
Copy link
Member

@yksflip yksflip commented Feb 17, 2023

This PR has the required changes for the ruby and rails upgrade, see #956

it'd make sense to first merge these:

  1. rswag api tests Replace apivore with rswag for api tests #969
  2. rebase master
  3. make ci run again
  4. run rubocop autofixes
  5. create issue for ruby 3 upgrade
  6. fix order matrix pdf generation error (sql injection possible?)
    Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "SUM(COALESCE(group_order_articles.result, group_order_articles.quantity))".This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql().
    
  7. check flaky tests
  8. do some more manual acceptance testing (real life scenarios)
  9. test production (docker) setup

  1. controller tests Controller Tests #970 Add home controller test #972 this isn't a blocker actually. would be nice for better coverage, but let's get the upgrade done first.

happy for any comments or ideas how we can proceed with this! :)

@yksflip yksflip mentioned this pull request Mar 5, 2023
@lentschi
Copy link
Contributor

lentschi commented Mar 10, 2023

@yksflip First of all: Thanks for all your work on this!! 😊

Then one question related to the reviewing process: Would it be possible to do the rubocop autofixes (changed due to the rubocop upgrade) in a separate PR? (That new PR could be easily merged without much review as rubocop's autofixes are fairly stable afaik. It would also make this PR - the actual ruby & rails upgrade - easier to review.)
Or is that simply not possible due to ruby syntax changes? (I didn't check)

@kidhab
Copy link
Contributor

kidhab commented Apr 22, 2023

How can we move forward with this and all related MR? I think it is more difficult to merge this if the development continues and the MRs and the master branch diverge more and more.
Maybe it's fine to solve the merge conflicts and just merge it? After all the opener worked in a team for about 6 months on the Foodsoft code and got support from the folks at Pragma Shift who work on a daily basis with RoR. Also there's a Foodsoft instance out there running for with the RoR update for some months now, isn't it?

Maybe we can ask some Foodcoops which are using the global hosting to use a testing instance with these MR to get more Feedback?

And maybe we can fund some work to solve all related conflicts if it's a lack of time?

@yksflip
Copy link
Member Author

yksflip commented Apr 24, 2023

hey!
thanks kidhab for the ping! You're absolutely right, and this is totally waiting on me right now. It's so great to see activity on the main branch and we should definetly get this merged soon! I'll get back on this next week!

@kidhab
Copy link
Contributor

kidhab commented May 1, 2023

With Ruby 2.7.8 the 2.7 series reached EOL. Maybe this is a good topic for the next community call.

@yksflip yksflip linked an issue May 6, 2023 that may be closed by this pull request
@yksflip yksflip marked this pull request as ready for review May 6, 2023 16:38
@yksflip
Copy link
Member Author

yksflip commented May 10, 2023

one question related to the reviewing process: Would it be possible to do the rubocop autofixes (changed due to the rubocop upgrade) in a separate PR? (That new PR could be easily merged without much review as rubocop's autofixes are fairly stable afaik. It would also make this PR - the actual ruby & rails upgrade - easier to review.) Or is that simply not possible due to ruby syntax changes? (I didn't check)

not sure if i understand correctly .. so there are still a lot rubocop violations that need to be autofixed .. I think it would make sense to do it in this PR, but in a seperate commit. I think for reviewing you can select what commits to display, so one could disable the rubocop commit ... The changes that are already in here are needed for syntax reasons I think.

@yksflip
Copy link
Member Author

yksflip commented May 10, 2023

With Ruby 2.7.8 the 2.7 series reached EOL. Maybe this is a good topic for the next community call.

why is software aging so fast? 😅
Should talk about it ... but hopefully going to ruby 3.1 shouldn't be a big deal i guess? (But I'd seperate it from this here ...)

chore: fix api test conventions

chore: rubocop -A spec/

chore: more rubocop -A

fix failing test

rubocop fixes

removes helper methods that are in my opinion dead code

more rubocop fixes

rubocop -a --auto-gen-config
@yksflip yksflip mentioned this pull request May 26, 2023
@yksflip
Copy link
Member Author

yksflip commented Jun 8, 2023

After some more testing I feel pretty confident about this changes. If nobody has any concerns, I'd go on with a optimistic merge ... @wvengen @paroga @lentschi

Copy link
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

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

Did a quick scan, looks ok to me in general. Most important thing is that it works in practice, including the different parts of the application. Since most coops don't use each and every feature, it's important to be fully aware of all nooks, ideally testing happens by multiple groups with different approaches. That said, I have confidence that this upgrade is fine, and would work, so a new release noting "you may want to wait upgrading until the next minor version" would probably work as well.

One question about CORS.

In any case, you've done a great work, thank you so much!

config/initializers/cors.rb Outdated Show resolved Hide resolved
Copy link
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

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

Super, thanks for your quick update! :)

@yksflip yksflip merged commit 4bfa87d into foodcoops:master Jun 9, 2023
@yksflip yksflip added this to the 4.8 milestone Jun 9, 2023
@kidhab kidhab mentioned this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Upgrade Rails to 6.x
6 participants