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

Allow Rails 6.1 #103

Merged
merged 1 commit into from
Feb 9, 2022
Merged

Allow Rails 6.1 #103

merged 1 commit into from
Feb 9, 2022

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 3, 2022

No description provided.

@agrare agrare requested a review from Fryguy as a code owner February 3, 2022 22:11
@agrare agrare requested a review from jrafanie February 3, 2022 22:27
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

are we testing these changes to the ancillary gemspecs or just merging them?

@agrare
Copy link
Member Author

agrare commented Feb 4, 2022

are we testing these changes to the ancillary gemspecs or just merging them?

Just because this does deal with Active Record persistence so directly I'd like to run this through with cross-repo actually using rails 6.1, that way we can fix any issues in this PR

@jrafanie
Copy link
Member

jrafanie commented Feb 4, 2022

Just because this does deal with Active Record persistence so directly I'd like to run this through with cross-repo actually using rails 6.1, that way we can fix any issues in this PR

I'm currently trying this out with rails 6.1 locally with a manual cross repo run.

jrafanie added a commit to jrafanie/manageiq-cross_repo that referenced this pull request Feb 4, 2022
In the event you're running several in a batch, it's nice to report on what
we're doing and not rely on ENV variables as some could be set via command
line.

The output looks something like this:

```
 Starting cross repo for:
  test repo: ManageIQ/manageiq-providers-lenovo
  core repo: ManageIQ/manageiq#21652
  gem repo:  ManageIQ/inventory_refresh#103
Fetching https://github.com/ManageIQ/manageiq-providers-lenovo/tarball/dfeb7c99963eac29e9eb9f8f2f78ba2c9c3ee866
```
@jrafanie jrafanie self-assigned this Feb 7, 2022
@Fryguy Fryguy mentioned this pull request Feb 9, 2022
@agrare agrare force-pushed the allow_rails_6_1 branch 2 times, most recently from eaa9096 to bcea63f Compare February 9, 2022 15:10
@agrare
Copy link
Member Author

agrare commented Feb 9, 2022

NOTE I've added rails 6.1 into the test matrix here

Gemfile Outdated
@@ -14,4 +14,6 @@ when "5.2"
gem "activerecord", "~>5.2.6"
when "6.0"
gem "activerecord", "~>6.0.4"
when "6.1"
gem "activesupport", "~>6.1.4"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gem "activesupport", "~>6.1.4"
gem "activerecord", "~>6.1.4"

Copy link
Member

@Fryguy Fryguy Feb 9, 2022

Choose a reason for hiding this comment

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

I think this repo actually uses both activerecord and activesupport, but only declares activerecord as a dependency...it probably should do both, and then also have both here as well.

@miq-bot
Copy link
Member

miq-bot commented Feb 9, 2022

Checked commit agrare@3a9b410 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@Fryguy Fryguy merged commit 1da03d5 into ManageIQ:master Feb 9, 2022
@Fryguy Fryguy assigned Fryguy and unassigned jrafanie Feb 9, 2022
@agrare agrare deleted the allow_rails_6_1 branch February 9, 2022 15:37
agrare added a commit that referenced this pull request Feb 9, 2022
Changed
- Run rubocop -A (#102)
- Switch from travis to github actions (#104, #105)

Added
- **BREAKING** Add back support for non-concurrent-safe batch strategies (#101)
- Add support for Rails 6.1 (#103)
- Add bundler-inject (#106)

Fixed
- Fix InventoryCollection missing cache returns (#107)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants