Rails 6.0 #503
Rails 6.0 #503
Conversation
manageiq-gems-pending.gemspec
Outdated
| @@ -20,8 +20,8 @@ Gem::Specification.new do |s| | |||
|
|
|||
| s.required_ruby_version = "> 2.4" | |||
|
|
|||
| s.add_runtime_dependency "activerecord", "< 5.3" | |||
There was a problem hiding this comment.
Can we apply https://github.com/ManageIQ/activerecord-id_regions/pull/18/files#r537735807 here as well?
There was a problem hiding this comment.
I don't see why we would, as this isn't a gem that ever needs to be released, and is only used by ManageIQ/manageiq, which now requires Rails 6. The only place where I think this does make sense is in gems that are released to rubygems.org or rubygems.manageiq.org which this is not.
(This is going to be a carbon copy answer that I will be pasting in all provider repos where this comment exists)
There was a problem hiding this comment.
I look at manageiq-gems-pending as a regular gem, but we don't push it to rubygems.org because it's not a single well thought out thing that is generically usable by other people. However, now that we have rubygems.manageiq.org, I would seriously consider pushing it there and acting like it is a real gem.
On that note, I don't know why there is a dependency on activerecord here. I don't see it required by anything in the repo, only mentioned in some documentation. I opened #504 to remove it.
activesupport is used in a couple places and I think we could loosen the dependency on it like we would in a regular gem.
@Fryguy thoughts on pushing this to rubygems.manageiq.org?
There was a problem hiding this comment.
Side note... These comments should not hold this up, I'm trying to make it easier to get your PRs (and similar ones in the future) merged.
There was a problem hiding this comment.
I never though about gems-pending being released, but that is a really interesting idea! I don't think we can release the rest of the plugin gems, but gems-pending is different as it's a bit more stable commit-wise and we'll likely only really keep removing things (semver wise, this gem is going to get a lot of major releases 😆 )
There was a problem hiding this comment.
That being said, that's even more of a reason to have a "real" release.
@Fryguy I would argue it is a reason to break out the code it does depend on into something that is less of a "junk drawer" repo and something more focused. I still tend to view this repo as "things we never really got around to making there own gems, or things that should have stayed in lib/ in ManageIQ/manageiq".
There was a problem hiding this comment.
I checked and it's actually 8 gems that use gems-pending - https://github.com/search?q=org%3AManageIQ+manageiq-gems-pending+filename%3AGemfile+archived%3Afalse&type=Code
There was a problem hiding this comment.
I look at manageiq-gems-pending as a regular gem, but we don't push it to rubygems.org because it's not a single well thought out thing that is generically usable by other people.
Fair, if we do want to go that route, I guess I am not opposed to the idea since it is one less git gem in our bundle, and this is used by a few things where having a non-git gem would be helpful.
That said, I am trying to avoid loose dependencies on stuff that is only intended to be used by "us", since it can just make things confusing when it comes to integrating this in ManageIQ and trying to bundle.
Edit: That said, I would rather go that #231 route and remove this gem entirely, which moving this to release on rubygems.manageiq.org just gets us further away from that.
There was a problem hiding this comment.
I think we're all on board with deleting it via #231 but it's a large effort that will likely take a while (already a multi-year effort). I also don't think that pushing it to rubygems.manageiq.org gets us any further from deleting it. And if it helps us loosen dependencies (and reduce bundler conflicts), I think that's a step forward.
|
Checked commit NickLaMuro@d2bf48c with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint |
|
@miq-bot remove-label unmergeable |
Links