Update Onebox#893
Conversation
42dbdb6 to
972758e
Compare
|
@glebm @timdiggins - CI used to run on PRs, but it appears that Travis no longer has their Can one of you sign up for a free travis-ci account on travis-ci.com and see if we can get builds going again? |
|
@rickychilcott thanks for letting me know. I've just tried to migrate or add them to travis-ci.com but it doesn't seem to work. I've reached out to travis-ci.com support and hopefully will be straightforward. |
| s.add_dependency 'kramdown', '>= 2.0.0' | ||
| s.add_dependency 'kramdown-parser-gfm' | ||
| s.add_dependency 'onebox', '~> 1.8', '>= 1.8.99' | ||
| s.add_dependency 'onebox', '~> 2.2' |
There was a problem hiding this comment.
my feeling is, as a library, we should prefer looser dependencies (ie. no ~> or <) unless we know there's a specific issue (e.g. it won't work with that version). Thus we could simplify this to '>= 1.8.99'. This allows users of the library to be specific about which version they want to use.
There was a problem hiding this comment.
That said I just read your comment "This PR is testing whether..." -- but you could test that by loosening the version and running it locally I think. (Sadly there doesn't seem to be any indication of what breaking changes Onebox 2.x brings in, if any: https://github.com/discourse/onebox/blob/master/CHANGELOG.md)
There was a problem hiding this comment.
PS I've run the specs locally against rails 6.0 x sqllite x onebox 2.2 and passes locally. Am in communication with travis support.
There was a problem hiding this comment.
Thank you, Tim. I haven't been able to get a local testing environment running locally, unfortunately. I'll try again next time I run into something.
I'm happy to make it '>= 1.8.99', which should get me over the CVE-2021-32740 issue.
I was also bummed to not see any details about what 2.x brings. I'm going to try a version of the app locally with this and deploy to a staging environment before I request a review.
Thank you for your work on this @timdiggins!
There was a problem hiding this comment.
If you want to make it >= 1.8.99, it should still run on travis with the latest version (because bundle install without a lockfile should I think install the latest version, even if travis is caching gems). Travis-ci.com is now building I think.
PS running specs locally is pretty straightforward, just a few steps -- https://github.com/thredded/thredded/#testing gives all the details (though in slightly the backwards order -- how to run first, then how to setup )
972758e to
1da3776
Compare
|
I was able to merge in master and change it to It successfully passes all test. I also tried a few URLs in a deployment to my staging environment and everything seems fine. Note that this is still not allowing us to go to addressable 2.8.0 (to resolve CVE-2021-32740), but I will work with the onebox team to cut a release of onebox that will allow that to happen. @timdiggins can you merge? |
|
Thank you @timdiggins. Onebox has cut a new gem now as well. All is right with the world! |
2 participants
To allow me to update the addressable gem (which is desired because of CVE-2021-32740), I need to update the onebox gem.
This PR is testing whether a onebox upgrade to 2.x will cause problems