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

Add Ruby 3.2, 3.3, HEAD to CI matrix #1042

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Add Ruby 3.2, 3.3, HEAD to CI matrix #1042

merged 1 commit into from
Jun 6, 2024

Conversation

s4na
Copy link
Contributor

@s4na s4na commented Mar 20, 2024

No description provided.

bundle install --quiet --path vendor/bundle

if [ "$(printf "%s\n" "3.2" "$RUBY_VERSION" | sort -V | tail -n1)" = "$RUBY_VERSION" ]; then
bundle config set --local path 'vendor/bundle'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@s4na can you explain the purpose of this conditional? would bundle config set + bundle install work the same way for all ruby versions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if this works the same way, then the conditional can be removed, right?

Copy link
Collaborator

@serggl serggl left a comment

Choose a reason for hiding this comment

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

@s4na left few more questions. I see you removed development dependencies version constraints. Although I see test suite is passing, this might broke at some time in the future. I would vote to still pin gems to some explicit versions (latest at the moment)

bundle install --quiet --path vendor/bundle

if [ "$(printf "%s\n" "3.2" "$RUBY_VERSION" | sort -V | tail -n1)" = "$RUBY_VERSION" ]; then
bundle config set --local path 'vendor/bundle'
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this works the same way, then the conditional can be removed, right?

@@ -30,13 +30,15 @@ Gem::Specification.new do |s|
s.executables = `git ls-files -- bin/*`.split("\n").map{ |f| File.basename(f) }
s.require_paths = ["lib"]

s.add_dependency 'rails', '>= 5'
s.add_dependency 'rails'
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was the restriction removed removed?

@s4na
Copy link
Contributor Author

s4na commented Jun 6, 2024

@serggl
Thank you for the review. I have made the necessary corrections.

@@ -25,12 +25,12 @@ Gem::Specification.new do |s|
s.executables = `git ls-files -- bin/*`.split("\n").map{ |f| File.basename(f) }
s.require_paths = ["lib"]

s.add_dependency 'rsolr', '>= 1.1.1', '< 3'
s.add_dependency 'rsolr', '>= 1.1.1', '< 2.6'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since RSolr 2.6 is causing errors, it needs to be addressed in a separate PR.

@s4na s4na requested a review from serggl June 6, 2024 12:15
build:
needs: skip_concurrent_build
if: ${{ needs.skip_concurrent_build.outputs.should_skip != 'true' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@s4na would you mind bringing this back? I'm totally fine merging this afterwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@serggl serggl merged commit 7bdd1ae into sunspot:master Jun 6, 2024
49 checks passed
@s4na
Copy link
Contributor Author

s4na commented Jun 6, 2024

Thank you

@rocket-turtle rocket-turtle mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants