-
Notifications
You must be signed in to change notification settings - Fork 162
Dependency-vulnerability: ruby-saml to 1.18, updates for tests to pass #263
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
Conversation
…n Rails 6.1 and 7.0 with ActiveSupport
…nt failures in tests in CI
.github/workflows/ci.yml
Outdated
| - spec/support/Gemfile.rails7 | ||
| - spec/support/Gemfile.rails7.1 | ||
| - spec/support/Gemfile.rails7.2 | ||
| - spec/support/Gemfile.rails8.0 |
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.
We probably don't need this one yet, since the latest release is still 8.0.x. We should keep the default Gemfile up to date with the latest Rails version. I'd be open to it being a symlink to this file, but either way only one should be present in the test matrix.
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.
Ok - the version in master currently tests multiple rails: 6.1, 7, and Gemfile. Any preference on which version? 7.1 is the lowest actively supported Rails as of today, should we test that, or should base still be 6.1? I bumped Gemfile version to rails 8.0 as you suggested. I'll merge that and the CI change in here when I hear back. Also - there are still odd failures with a test here or there related to Capybara/Selenium/Headless Chrome ( a random 1/136 Selenium::WebDriver::Error::UnknownErrortest failure on a different test each time, despite the test passing when individually run). I am seeing this being reported elsewhere as well so I'm going to try adding some additional Chrome options to the config that are working for others. Sorry about that, everything passed in my fork branch before I created this PR.
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.
Sorry I wasn't clear! We should test all supported versions of Rails:
Gemfileshould have the latest version of Rails, in this case 8.0.xGemfile.rails7.2should have Rails 7.2.xGemfile.rails7.1should have Rails 7.1.x
And we can delete all the other Gemfiles (6.x, 8.0).
…onsistent failures waiting for page load before checks
|
@adamstegman I changed CI tests to run only against one Rails (7.1) in the test matrix and updated the Gemfile to use Rails 8 as you suggested. All tests pass locally and against a PR in my fork's master branch as well. If any individual job fails, please re-run the failure manually. There is currently an issue with the Chromium webdriver where occasionally Chrome does not wait for the browser to load before checking the URL, causing otherwise passing tests to fail. |
adamstegman
left a comment
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.
Thanks for taking the time to fix these tests! I'm not too worried about occasional flakes in this project, because it doesn't get frequent updates, so even if we have to re-run a few we can still merge.
spec/support/idp_template.rb
Outdated
|
|
||
| # Rails 6.1 and 7.0 bug where ActiveSupport uses logger but does not require it. Concurrent ruby used to include | ||
| # logger which masked the bug but stopped including it in 1.3.5. Fixed in rails >=7.1. | ||
| if defined?(Rails) && Rails.version < '7.1' |
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.
If we're not testing < 7.1, we can remove this and related branches. Happy to do this cleanup later though!
|
Hi, when we can expect a release of the gem with this update @adamstegman ? |
|
@adamstegman All of these tests pass in my fork PR and hopefully will pass on a re-run - looks to still be the intermittent chromium issue. I had accidentally left an uneeeded Rails 8 issue workaround in the sp_template due to not saving, removed that. Added back in Rails 7.2 tests and cleaned up various workarounds for for older ruby and rails versions in the test configuration that are no longer needed. |
|
Hi, When can we expect a release with this update? |
|
@camol You shouldn’t be blocked on this change! There’s nothing in this PR blocking you from upgrading your own dependency. |
This PR upgrades ruby-saml to 1.18 to address vulnerabilities in the dependency. It also contains some minor changes so that CI testing passes on Rails > 7.1. Additional CI config changes are discussed below. I should note that ruby-saml 1.12.4 also addresses the vulnerability, but only ~1.12.4 and >=1.18 or above are patched.
I made some choices in regard to CI testing in this PR that should be noted and I'll provide my reasoning, however these should probably be discussed.