-
Notifications
You must be signed in to change notification settings - Fork 11
Update CI workflow matrix #72
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
588fadf to
7435e18
Compare
7435e18 to
6ce1e90
Compare
justinhoward
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 @joshuay03 ! Can you talk a bit more about the changes you're making and some of those decisions?
.github/workflows/ci.yml
Outdated
| services: | ||
| redis: | ||
| image: redis | ||
| image: redis:${{ matrix.redis }} |
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.
matrix.redis is actually the gem version, not the server version.
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.
Ah yes I see that now, that simplifies things a bunch if we just want to use the latest server version.
we just want to use the latest server version
Should we do the same for opensearch and elasticsearch?
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.
The reason I added the different OpenSearch/ElasticSearch versions was because there were some backwards-compatibility breaks introduced by Elastic around the time they switched their licensing. I had to work around those issues.
| ruby: ['2.3', '2.4', '2.5', '2.6', '2.7', '3.0', '3.1', '3.2', '3.3', '3.4', head, jruby-head, truffleruby-head] | ||
| redis: ['3', '4', '5', '6', '7', '8'] | ||
| search: [ | ||
| ['opensearch-ruby:1', 'opensearchproject/opensearch:1'], | ||
| ['opensearch-ruby:2', 'opensearchproject/opensearch:2'], | ||
| ['opensearch-ruby:3', 'opensearchproject/opensearch:3'], | ||
| ['elasticsearch:7', 'elasticsearch:7.17.28'], | ||
| ['elasticsearch:8', 'elasticsearch:8.18.2'], | ||
| ['elasticsearch:9', 'elasticsearch:9.0.2'] | ||
| ] |
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.
This matrix is much too large to actually run. We'll want to pair it down to just a sampling of the things we want to support.
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.
the things we want to support
Yes this is what I don't have context on. So we've got rubies 2.3 - 3.4 (as per the gemspec) and head versions just to catch incompatibilities early, and redis gem majors 3 and 4. Would you like support the last three major versions of opensearch and elasticsearch? Or just two like redis?
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.
FWIW the matrix should be good now, I'm testing it in my fork: joshuay03#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.
and redis gem majors 3 and 4
If this is what we're going with, I can update:
Line 32 in b3921b3
| spec.add_development_dependency 'redis', '>= 3.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.
Would you like support the last three major versions of opensearch and elasticsearch? Or just two like redis?
Went with two in my fork to leave capacity in the matrix for cluster enabled runs: joshuay03#4
| - uses: ruby/setup-ruby@v1 | ||
| with: | ||
| ruby-version: '2.7' | ||
| ruby-version: '3.4' |
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.
I'd rather keep these on the lower supported versions I think. Is there a reason you want to change it?
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.
Just thought I'd use the latest ruby, happy to revert back to 2.7.
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.
My thinking is that unless it's unreasonable, we should support dev tooling on our supported ruby versions. One way to check if that's the case is by using the older ruby to run it in CI. I think there's a limitation of Rubocop that prevents using the oldest version, and that's how I ended up with 2.7.
Of course, we might consider dropping support for some older ones, but hasn't been important yet since older support has been trivial.
98b8633 to
2c25553
Compare
| env: | ||
| discovery.type: single-node | ||
| plugins.security.disabled: ${{ contains(matrix.search[1], 'opensearch') && 'true' || '' }} | ||
| OPENSEARCH_INITIAL_ADMIN_PASSWORD: ${{secrets.OPENSEARCH_INITIAL_ADMIN_PASSWORD}} |
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.
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.
Something to keep in mind when setting the secret:
Please re-try with a minimum 8 character password and must contain at least one uppercase letter, one lowercase letter, one digit, and one special character that is strong. Password strength can be tested here: https://lowe.github.io/tryzxcvbn
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 can just choose something and stick it in the file directly right? There's no reason it needs to be secret.
5b39156 to
8cb392e
Compare
My main goal here is to have a matrix that's explicit about all supported dependencies, and excludes incompatibilities as needed, so all permutations are covered. Edit: I've got everything except for the ruby head runs green in joshuay03#1. Not sure what's going on there but my guess is it's an upstream issue. |
2757c7f to
cd9f8f1
Compare
65a752c to
24187fa
Compare
02ad5f1 to
15700fd
Compare
| - uses: ruby/setup-ruby@v1 | ||
| with: | ||
| ruby-version: '2.7' | ||
| ruby-version: '3.4' |
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.
My thinking is that unless it's unreasonable, we should support dev tooling on our supported ruby versions. One way to check if that's the case is by using the older ruby to run it in CI. I think there's a limitation of Rubocop that prevents using the oldest version, and that's how I ended up with 2.7.
Of course, we might consider dropping support for some older ones, but hasn't been important yet since older support has been trivial.
| spec.add_development_dependency 'connection_pool', '~> 2.0' | ||
| spec.add_development_dependency 'json' | ||
| spec.add_development_dependency 'redis', '>= 3.0' | ||
| spec.add_development_dependency 'redis', '>= 4.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.
Do we need to drop support for 3? That's going to be a breaking change.
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.
3 wasn't running in CI, so I opted to drop it instead of adding it there. But you're right, that would warrant a separate PR.
|
|
||
| def build_client(**options) | ||
| patched_module::Client.new(options) | ||
| if Gem.loaded_specs['opensearch-ruby'] |
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.
This looks reasonable, but patched_module should already do the right thing here right?
| ['elasticsearch:8', 'elasticsearch:8.18.2'], | ||
| ['elasticsearch:9', 'elasticsearch:9.0.2'] | ||
| ] | ||
| exclude: |
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.
This is still going to be a pretty big matrix with a lot of CI time, as opposed to the include approach, that just takes a sample of the combinations of supported versions. I'm going to run CI and see what it looks like.
|
@justinhoward Thank you for reviewing, but I think I'm going to close this and leave CI changes to you, and only focus on redis cluster support (#74). At the very least though, I think it would be good to have the latest rubies in CI. |
|
Okay, thanks for your time! |
WIP until green.Looks like I need workflow approval so will mark as ready and iterate.