-
Notifications
You must be signed in to change notification settings - Fork 13
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
3384 – update rails to ~> 6.1.7 #365
Conversation
@hartsick I did the bundle update, seems like there wasn't javascript stuff? Though I'm really not sure if I'm missing something. But the build errored, I'll try to look into this again later. |
@vasconsaurus Yep! Bundle just updates Ruby dependencies. If we need Javascript updates you'll still need to run the Javascript commands you mentioned in the ticket. Also, sorry for delay on response - had this typed out last week and forgot to press comment apparently! |
@hartsick sorry I misspoke, I meant it doesn't seem like there is any javascript stuff to update, but I'm not sure. |
@vasconsaurus no worries! were you able to build the docker image and spin up the container locally? wondering if we can reproduce the problem outside of CI |
delayed extensions was removed in sidekiq 7 and that breaks things for us https://github.com/sidekiq/sidekiq/blob/main/Changes.md#640
0da8447
to
e7b7982
Compare
@hartsick ✨ Updates ✨ (I think I'm going to add this to the ticket as well)
|
There seems to be a super old monkey patch in postrank that messes with the normalize method, starting at 2.8.2 version. Until postrank fixes that we can only go up to 2.8.1 relevant links: sporkmonger/addressable#513 sporkmonger/addressable#506 postrank-labs/postrank-uri#49
Code Climate has analyzed commit 6af1098 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 98.0% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go if tests pass! I didn't see any major upgrade of concern (like, Sidekiq would be one, so I'm glad we're guarding against that).
ran integration tests, they passed 🎉 so I'm merging this one @caiosba should we open a ticket to do the |
@vasconsaurus sure, but no need to schedule it yet. Thanks! |
Description
There’s a CVE for actionpack in Pender that should be resolved by us upgrading Rails to 6.1.74 (or just doing ~>6.1.7 and upgrading)
References: cv2-3384
How has this been tested?
Ran a request to
pender
, like the one below, got the parsed response 🎉Things to pay attention to during code review
Although a tiny change, a few things broke:
delayed extensions
anymore, I kept it under that. However, we use that in one place, it doesn't look like it will be super hard to upgrade(🤞). Seems to be setup here (03_sidekiq.rb#L27) and used here(media_archive_org_archiver.rb#L28).uri.normalize
relevant links / sidekiq's upgrade
relevant links / addressable issue