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

Update dependency versions inside gemspec #413

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apuntamb
Copy link

@apuntamb apuntamb commented Feb 26, 2019

This PR updates the versions of following dependencies which are needed by foreman_azure_rm has started to point to the latest fog-azure-rm (0.4.9).
Foreman compute resources are in process to be bumped to refer to fog-2.x and fog-core 2.x and the versions currently being used for the selected dependencies in this PR, show certain conflicts with other gems in terms of version requirements. Preferred fog-core version for fog-azure-rm in this case is >=2.x. For the other three dependencies fog-json, azure-storage, mime-types, newer versions satisfy the version specifications too.

@ares
Copy link

ares commented Feb 26, 2019

LGTM

@apuntamb
Copy link
Author

@maham-nazir-confiz Could you please review this change?

@apuntamb
Copy link
Author

apuntamb commented Mar 1, 2019

Any updates on this? @maham-nazir-confiz

@ShimShtein
Copy link

ACK from me.
@plribeiro3000, if we can get this in, we could move foreman-azure-rm to use the latest core (2.1.0).

@plribeiro3000
Copy link
Member

@ShimShtein I'm on it. Sorry for the delay.

@plribeiro3000
Copy link
Member

LGTM.

@ShimShtein
Copy link

@maham-nazir-confiz can you please update about the status of this PR? It's critical for https://github.com/theforeman/foreman_azure_rm, since Foreman project moves to fog-core 2.1 and the plugin will not be able to function with current version of fog-azure-rm.
This change was already tested in Foreman and it looks good, the system functions as expected. If you require more testing, or any other requirement, please tell us, so we will be able to comply for those requirements.

@maham-nazir-confiz
Copy link
Contributor

@ShimShtein

Hi,
I'm sorry but we cannot merge this PR. This PR is not be backward compatible and will cause breakage for any other system that uses this gem.

Regards

@ShimShtein
Copy link

@maham-nazir-confiz, first thanks for responding to this. The response is much appreciated.
From what I can see, latest fog 1.x core is from August 2017 (More than a year old).
How long do you wish to support fog 1.x core, especially when other fog plugins already moved to core 2.1. Right now I already know about GCE, AWS, vSphere and OpenStack with more to join soon.

Copy link

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

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

From looking at other dependencies, fog-core was the only one that is not backwards compatible.

@maham-nazir-confiz, what do you say about this?

@@ -22,8 +22,8 @@ Gem::Specification.new do |spec|
spec.add_development_dependency 'minitest', '~> 5.8.4'
spec.add_development_dependency 'simplecov'
spec.add_development_dependency 'codeclimate-test-reporter' , '~> 1.0.0'
spec.add_dependency 'fog-core', '~> 1.43'
spec.add_dependency 'fog-json', '~> 1.0.2'
spec.add_dependency 'fog-core', '~> 2.1.0'

Choose a reason for hiding this comment

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

Let's try and make it backwards compatible - I suggest removing version restriction.

Suggested change
spec.add_dependency 'fog-core', '~> 2.1.0'
spec.add_dependency 'fog-core'

@apuntamb
Copy link
Author

@maham-nazir-confiz As suggested by @ShimShtein , I have tested the above changes. In addition to those, I assume we can also remove version restriction on fog-json so that there will not be any hard dependency on fog-core being migrated to 2.0. Please let us now your thoughts on this.
It would be great if you can review it with these changes. I guess that might solve our problem of backward compatibility.

@@ -32,7 +32,7 @@ Gem::Specification.new do |spec|
spec.add_dependency 'azure_mgmt_traffic_manager', '~> 0.9.0'
spec.add_dependency 'azure_mgmt_sql', '~> 0.9.0'
spec.add_dependency 'azure_mgmt_key_vault', '~> 0.9.0'
spec.add_dependency 'azure-storage', '= 0.11.5.preview'
spec.add_dependency 'azure-storage', '~> 0.11.preview'
Copy link

Choose a reason for hiding this comment

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

Can we use the latest version 0.15.0.preview here?

Choose a reason for hiding this comment

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

@bguban, ~> 0.11.preview will take 0.15.0.preview too.

Choose a reason for hiding this comment

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

~> 0.11.preview means >= 0.11 including preview and < 1.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how it will take 0.15.0.preview as well? When I install the gem with these dependencies, it just shows as 0.11.preivew

Choose a reason for hiding this comment

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

According to thoughtbot (specifically the second part - ~> 1.1 example) this will result with >= 0.11 and < 1.0 as I have mentioned before. According to rubygems pattern guide

Using ~> with prerelease versions will restrict you to prerelease versions only.

Which should be good, since Azure only releases .preview versions to rubygems

We already tested it, and 0.15.preview was pulled.

Copy link

Choose a reason for hiding this comment

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

@maham-nazir-confiz, @ShimShtein maybe as possible solution is just to increment the major version of the fog-azure-rm and use the latest versions of dependent gems in it. If somebody needs backward compatibility they would use the current version, who don't need it could use the newer version of the gem.

I agree, upgrading to later gem versions would resolve much of these issues.

Copy link
Author

Choose a reason for hiding this comment

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

@bguban This can be a possible solution.
@maham-nazir-confiz your thoughts on @bguban 's comment?

Choose a reason for hiding this comment

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

@maham-nazir-confiz, Actually it depends on the user to limit nokogiri version. I mean the user can limit nokogiri version in his own Gemspec file like this:

gem 'fog-azure-rm' # require our library
gem 'nokogiri', '<=1.9.1' # if the user wants ruby 2.0 compatibility

azure-storage does not require newer 'nokogiri', it sets only lower limit requirement. It's up to user to set up higher limit, if the user needs it.

Other option would be adding an if condition into fog-azure-rm.gemspec, but I am not sure it will be parsed well by rubygems.

Copy link
Author

Choose a reason for hiding this comment

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

@ShimShtein That's right, users can set the upper limit of nokogiri, if required, in their own gemspec.
@maham-nazir-confiz What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

@maham-nazir-confiz Could you please let us know if the approach suggested against nokogiri failure works for you?

@ShimShtein
Copy link

ACK from me, it's also backwards compatible.

spec.add_dependency 'fog-core', '~> 1.43'
spec.add_dependency 'fog-json', '~> 1.0.2'
spec.add_dependency 'fog-core'
spec.add_dependency 'fog-json'
Copy link
Author

Choose a reason for hiding this comment

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

@maham-nazir-confiz I have removed the version dependency here. Could you please take a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll get back to you after reviewing the changes.

Regards

Copy link
Author

Choose a reason for hiding this comment

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

@maham-nazir-confiz Any updates on this?

@maham-nazir-confiz
Copy link
Contributor

@apuntamb
I'll have a definitive answer for you in a couple of days. Running my tests right now.

Regards

@apuntamb
Copy link
Author

@maham-nazir-confiz
OK sure, thanks for the update.

@bguban
Copy link

bguban commented Oct 2, 2019

@maham-nazir-confiz, any news about this PR?
I started updating my app to Rails 6 and found that rails uses actiontext gem which uses nokogiri. And it's required version is >= 1.8.5.
So now I'm blocked by fog-azure-rm and its dependency on the old nokogiri version.

@maham-nazir-confiz
Copy link
Contributor

@bguban
Hi. We are actually in the phase of upgrading the fog-azure-rm gem to the latest SDK versions. We're doing the POC on it right now. Once it's green, we'll go ahead with the changes.

Regards

@stanhu
Copy link

stanhu commented Jul 9, 2020

@maham-nazir-confiz Are you still working on this? I'd love to get fog-azure-rm working with fog-core v2.x. Do you want others to take over maintenance of this project?

@frantisekrokusekpa
Copy link

Any updates on this?

@stanhu
Copy link

stanhu commented Apr 27, 2021

For those of you interested, I forked this gem and made numerous fixes: https://gitlab.com/gitlab-org/gitlab-fog-azure-rm

@ghost
Copy link

ghost commented Oct 7, 2021

@stanhu Hi Stan, thank you for taking initiative in this topic and maintaining this fork. As you may have noticed the azure Ruby SDK is in the process of being retired (except for the storage gems): https://github.com/Azure/azure-sdk-for-ruby#important-announcement

This includes the ms_rest_azure gem which still seems to be a dependency of your fork and would need replacement (see migration guide: https://github.com/Azure/azure-sdk-for-ruby/blob/master/docs/README.md). Are there any plans on GitLab's side to keep supporting the gitlab-fog-azure-rm fork or do you want to move to a different solution? We're asking because we're currently evaluating hosting files on Azure using Carrierwave and are wondering if we can still count on this being supported via the fog adapter/API. It is not looking too promising so far as even the 'supported' gems seem to be in a somewhat difficult spot right now (Azure/azure-storage-ruby#194 (comment) and Azure/azure-storage-ruby#196)

Cheers!

@stanhu
Copy link

stanhu commented Oct 7, 2021

@sebnjk I can't promise anything, but I'd imagine we'll have to support the gem for a while since GitLab has customers that rely on that gem.

I personally would like to migrate away from CarrierWave and Fog in favor of a solution that focuses on object storage and uses the SDKs directly as much as possible, which fog-aws does not. https://github.com/google/go-cloud is a good model in Go; I'd like to see something like that in Ruby.

Note that the fork only supports file transfers; I stripped all the other cloud-related functionality.

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.

9 participants