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

[CI] Test against rails 6.1 #332

Merged
merged 5 commits into from
Jul 11, 2022
Merged

[CI] Test against rails 6.1 #332

merged 5 commits into from
Jul 11, 2022

Conversation

waghanza
Copy link
Contributor

Hi,

This PR adds rails6 in CI matrix.

As rails6 require ruby >2.5, I've also excluded jruby from rails6 tests.

Regards,

@waghanza waghanza changed the title Add rails6 on CI [CI] Test against rails 6.1 Dec 8, 2021
@ivy
Copy link
Collaborator

ivy commented Dec 11, 2021

Hey @waghanza - Thanks for updating your PR! 🙌 It looks like you’ve just got a couple of Rubocop violations:

Offenses:

gemfiles/rails_6.1.gemfile:1:1: C: [Correctable] Style/FrozenStringLiteralComment: Missing frozen string literal comment.
source 'https://rubygems.org'
^
gemfiles/rails_6.1.gemfile:8:3: C: [Correctable] Bundler/OrderedGems: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem actionpack should appear before activerecord.
  gem 'actionpack', '~> 6.1.0'
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
gemfiles/rails_6.1.gemfile:14:3: C: [Correctable] Bundler/OrderedGems: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem jrjackson should appear before logstash-event.
  gem 'jrjackson', '~> 0.2.9', platforms: :jruby
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

46 files inspected, 3 offenses detected, 3 offenses auto-correctable 

I’ll fix them and get this merged in next time I’m in front of a computer unless you get to it first!

@waghanza
Copy link
Contributor Author

Done @ivy

Maybe worth using rubocop-rspec also

@iloveitaly
Copy link
Collaborator

@waghanza tests are still failing. Want to give it another try? Would love to get this change in.

@waghanza
Copy link
Contributor Author

@iloveitaly @ivy Not sure what is going on ...

@iloveitaly
Copy link
Collaborator

@waghanza still failing, not sure what is going on here.

@waghanza
Copy link
Contributor Author

Ruby 2.7 / Rails 6 fails and prevent other CI to run

@ytkg
Copy link
Contributor

ytkg commented Jun 26, 2022

@waghanza I don't know for what reason it was failing, but #352 has been merged, it may succeed!

@iloveitaly
Copy link
Collaborator

@waghanza @ytkg could either of you rebase this branch on master and see if it's passing now?

@waghanza
Copy link
Contributor Author

@iloveitaly rebased, the CI requires an approval

# logstash does not release any gems on rubygems, but they have two gemspecs within their repo.
# Using the tag is an attempt of having a stable version to test against where we can ensure that
# we test against the correct code.
gem 'logstash-event', git: 'https://github.com/elastic/logstash', tag: 'v1.5.4'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iloveitaly what is the rational behind using this version ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea :) If this needs to change, lets do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC (it's been a while), some things changed in the Logstash codebase that removed a class that's depended on in one of Lograge's adapters.

@waghanza waghanza requested review from iloveitaly and ivy June 29, 2022 18:55
gemfiles/rails_6.1.gemfile Outdated Show resolved Hide resolved
@iloveitaly iloveitaly merged commit 49267f9 into roidrage:master Jul 11, 2022
@waghanza waghanza deleted the rails6 branch July 12, 2022 18:01
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.

4 participants