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 dependencies to match new fog-google #17258

Merged
merged 1 commit into from
May 30, 2018

Conversation

tumido
Copy link
Member

@tumido tumido commented Apr 6, 2018

In order to upgrade fog-google in google provider, it is required to upgrade the mime_types in our core as well.

Dependency chain:

Related: ManageIQ/manageiq-providers-google#54

@tumido
Copy link
Member Author

tumido commented Apr 6, 2018

@miq-bot add_label wip

@tumido tumido force-pushed the update_deps_for_google branch from e50779c to d08636c Compare May 16, 2018 07:50
@miq-bot
Copy link
Member

miq-bot commented May 16, 2018

Checked commit tumido@d08636c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@tumido tumido changed the title [WIP] Update dependencies to match new fog-google Update dependencies to match new fog-google May 25, 2018
@tumido
Copy link
Member Author

tumido commented May 25, 2018

@miq-bot remove_label wip
Parent/dependent PR is ready for review, removing WIP

@miq-bot miq-bot removed the wip label May 25, 2018
@agrare
Copy link
Member

agrare commented May 30, 2018

cc @bdunne @NickLaMuro is it okay to just change the version in the gemspec for our mime-types-redirector?

@NickLaMuro
Copy link
Member

@agrare eww.... I would have to look. There might be some APIs used in this new version that might not match. I can look in a bit.

Thanks for the heads up.

@agrare
Copy link
Member

agrare commented May 30, 2018

@NickLaMuro yeah that's what I was worried about 😟

@NickLaMuro
Copy link
Member

@agrare Okay, so quick grep through the google-api-client codebase showed that MIME::Types is only called here:

https://github.com/google/google-api-ruby-client/blob/38e271cf/lib/google/apis/core/upload.rb#L58

And it only uses .of, which we have aliased here:

https://github.com/ManageIQ/manageiq/blob/f2774956/mime-types-redirector/lib/mime-types.rb#L13

I need to check if the mime-types gem itself has had any changes to MIME::Types.of as well. Also, will just double check that the fog-google gem isn't using MIME::Types in any form either.

@NickLaMuro
Copy link
Member

NickLaMuro commented May 30, 2018

@agrare @tumido Okay, I checked the changes between mime-types 2.6.1 and 3.x, and the API for MIME::Types.type_for (MIME::Types.of aliases to that method) has not changed much, and nothing seems to use MIME::Types.[] anyway, which changed to keyword arguments instead of an trailing (optional) options hash.

I ✌️ think ✌️ that you will be okay.

@NickLaMuro
Copy link
Member

Also, fog-google does not use mime-types directly either.

@agrare
Copy link
Member

agrare commented May 30, 2018

Thanks for digging into this @NickLaMuro !
Given that the whole google provider is currently broken I'm thinking we just merge this and fix any side-effects as they come up.

@agrare agrare merged commit 437ea65 into ManageIQ:master May 30, 2018
@agrare agrare added this to the Sprint 87 Ending Jun 4, 2018 milestone May 30, 2018
@agrare agrare self-assigned this May 30, 2018
@tumido
Copy link
Member Author

tumido commented May 30, 2018

@NickLaMuro yes, I confirm, this change is required just because it's google-api-client's dependency, the fog-google itself doesn't need it by any means.

@NickLaMuro
Copy link
Member

@tumido Just to clarify, I wasn't under the impression that fog-google had a direct dependency in it's gemspec for mime-types, but I am always cautious of developers using implicit dependencies in their project's code base.

Take for example the rspec-rails gem: No where in the gemspec is there a mention of ActiveRecord, but they clearly have a file for configuring just that. Obviously in this case, they are being very defensive and this is probably on purpose, but this illustrates my point that a gemspec's requirements isn't enough to justify if a dependency issue might be fine or not.

Plus, I am pessimistic about other developers and their code... call me a curmudgeon... ¯\_(ツ)_/¯

@tumido
Copy link
Member Author

tumido commented May 30, 2018

@NickLaMuro, I know, my dear curmudgeon! 😃 It's always better to be super cautious and get to the root of things...

I honestly don't understand the background here (why the gem is overridden this way etc.) so I'm totally ok with any suggestion and raised concerns.

Actually I'm not happy at all I had to do this PR since I don't see that far about what all the outcomes can be - especially since it's a permanent change and some other gem can resolve to this 3.0 version and try to leverage it's functionality, which might be missing... And all this just because one call in google-api-client.

I totally can see, how you got that curmudgeon name, haha.

@NickLaMuro
Copy link
Member

I honestly don't understand the background here (why the gem is overridden this way etc.)...

Oh, glad you asked

⚠️ WARNING: Book length explanation detected! ⚠️


So this PR against the rest-client gem is a good example of the problem:

rest-client/rest-client#557 (rebased by Jason here)

Effectively, just to load/require mime-types, it was a cost of 10MB, which is kind of absurd. What is worse, is that simple PR has been stale for over a year and a half, and rest-client is an implicit dependency for our app. Also, because we were using the mail gem as well, it meant we were now bringing in two different "mime types gems" to do the same thing.

The point of this wrapper is effectively to leverage the lighter weight "mime gem" in our application, which is historically a large ruby process just by default. The wrapper effectively takes the mini_mime gem, and wraps it in a mime-types gem interface, and allows gems that depend on mime-types to resolve just fine, without actually needing the real gem itself. This was implemented in this PR... though I realize that my terse and comical description at the time and lack of context is now causing issues.

(frantically goes and updates the PR description with links and context...)

Unfortunately, there are many downsides to this approach, and your confusion (along with pretty much everyone else's) is chiefly among them. But, you know, that whole "hindsight" thing and whatnot...

@simaishi
Copy link
Contributor

simaishi commented Jun 4, 2018

@agrare @NickLaMuro I assume this can be gaprindashvili/yes, but double-checking...

@agrare
Copy link
Member

agrare commented Jun 4, 2018

@simaishi I don't have a problem with it but we should make sure gaprindashvili will bundle with this version bumped before backporting.

@simaishi
Copy link
Contributor

simaishi commented Jun 4, 2018

yup, we're bringing ManageIQ/manageiq-providers-google#54 into Gaprindashvili.

simaishi pushed a commit that referenced this pull request Jun 4, 2018
@simaishi
Copy link
Contributor

simaishi commented Jun 4, 2018

Gaprindashvili backport details:

$ git log -1
commit 184568b8cfead13a10efd5c5b2d68b9c01ab375f
Author: Adam Grare <[email protected]>
Date:   Wed May 30 15:07:18 2018 -0400

    Merge pull request #17258 from tumido/update_deps_for_google
    
    Update dependencies to match new fog-google
    (cherry picked from commit 437ea6546e65f8870ef3169d22dafb211a966f8c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1585821

@tumido tumido deleted the update_deps_for_google branch June 26, 2018 14:18
@tumido
Copy link
Member Author

tumido commented Aug 27, 2018

@miq-bot add_label fine/yes

@simaishi Adding this to fine since it's a dependency for the ManageIQ/manageiq-providers-google#54

simaishi pushed a commit that referenced this pull request Aug 29, 2018
Update dependencies to match new fog-google
(cherry picked from commit 437ea65)
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit e6587a1c239ba975b15f8f84a8b8232f1b76fef3
Author: Adam Grare <[email protected]>
Date:   Wed May 30 15:07:18 2018 -0400

    Merge pull request #17258 from tumido/update_deps_for_google
    
    Update dependencies to match new fog-google
    (cherry picked from commit 437ea6546e65f8870ef3169d22dafb211a966f8c)

tumido pushed a commit to tumido/manageiq that referenced this pull request Aug 31, 2018
Update dependencies to match new fog-google
(cherry picked from commit 437ea65)
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