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

rename client's proxy option #73

Merged
merged 2 commits into from
Oct 4, 2018

Conversation

AlexanderZagaynov
Copy link

@AlexanderZagaynov AlexanderZagaynov commented Sep 14, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1623862

In a recent google-api-ruby-client gem versions option proxy now named as proxy_url:
https://github.com/googleapis/google-api-ruby-client/blob/8faa43b9f7e6b06cc820df4bcffb384b988e33d1/lib/google/apis/options.rb#L21

This PR align that option name accordingly.

P.S. There is one more PR for that: fog/fog-google#418
but current one can be merged without waiting.

@AlexanderZagaynov
Copy link
Author

@tumido @juliancheal please review

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Nice one! 👍 Looks great! I love these kinds of fixes.

However I would wait for the fog-google PR to get merged and then update this PR with fog-google version upgrade (so we can be sure the gem is resolved with the fix in place).

@juliancheal
Copy link
Member

This PR looks great @AlexanderZagaynov. Although I'm not sure what relation it has the the fog-google PR. Sorry for misunderstanding.

@AlexanderZagaynov
Copy link
Author

AlexanderZagaynov commented Sep 18, 2018

@juliancheal your previous commit to fog-google was borked by some update
(I've put information in that PR).
Those two PRs together should solve mentioned BZ issue.

@juliancheal
Copy link
Member

@AlexanderZagaynov ah that makes sense.

@tumido I thought if we merge this one, at least our code is correct?

@AlexanderZagaynov
Copy link
Author

@juliancheal Tom's proposal to wait for fog-google gem update makes sense.

@JPrause
Copy link
Member

JPrause commented Sep 18, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Sep 18, 2018

@AlexanderZagaynov if this can be backported, can you add the gaprindashvili/yes label.

@AlexanderZagaynov
Copy link
Author

@miq-bot add_label gaprindashvili/yes

@juliancheal
Copy link
Member

@miq-bot assign @agrare

@juliancheal
Copy link
Member

@agrare are we good to merge?

@juliancheal
Copy link
Member

Hi @juliancheal let's wait, as Tom said: #73 (review)

Oh my mistake. I thought we were going ahead with this regardless. I must of imagined that :)

@AlexanderZagaynov
Copy link
Author

@tumido @juliancheal fog-google PR was merged, however wasn't released as a version yet.
Gem maintainer (Artem @Temikus) told me that he going to release it in a days, so I just fixed that gem version in our gemspec. We can wait merge till fog-google release, or we can simply point it to github repo, if we're in hurry.

@agrare
Copy link
Member

agrare commented Sep 24, 2018

I think we're going to have a problem with moving to the newer fog-google, on master they have spec.add_dependency "fog-core", ">= 2.0", "<= 2.1.0" where on v1.7.1 they just have spec.add_dependency "fog-core".

We're currently using fog-core 1.45 I believe and are in the process of updating other provider gems to support fog-core 2.* but were going to wait until after hammer was branched to merge these.

This would require we pull those back to gaprindashvilli.

Ideally the fog-google maintainer could pull this change to v1.7.2 and leave the gemspec requirements the same.

@AlexanderZagaynov
Copy link
Author

AlexanderZagaynov commented Sep 24, 2018

@agrare I discussed it with Artem, and he told me, he is going to do that at v2.0 only

@agrare
Copy link
Member

agrare commented Sep 24, 2018

Okay awesome thanks @AlexanderZagaynov

@miq-bot
Copy link
Member

miq-bot commented Sep 24, 2018

Checked commits AlexanderZagaynov/manageiq-providers-google@bc7ba98~...7e060d9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Temikus added a commit to Temikus/fog-google that referenced this pull request Oct 4, 2018
Relaxing the lower version constraint to be less restrictive in combinations with other providers.

Reference: ManageIQ/manageiq-providers-google#73
@AlexanderZagaynov
Copy link
Author

@tumido @juliancheal @agrare done, can merge now

@agrare
Copy link
Member

agrare commented Oct 4, 2018

Awesome thanks a lot @Temikus! We're upgrading to newer fog-core in this release but need this fix on older releases. Really appreciate your help.

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

@juliancheal
Copy link
Member

Well done on getting it all committed in fog too!

@agrare agrare merged commit e508156 into ManageIQ:master Oct 4, 2018
agrare added a commit that referenced this pull request Oct 4, 2018
@agrare agrare added this to the Sprint 96 Ending Oct 8, 2018 milestone Oct 4, 2018
@AlexanderZagaynov
Copy link
Author

@miq-bot add_label hammer/yes

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

simaishi commented Oct 4, 2018

Hammer backport details:

$ git log -1
commit af5dfef52fe510f849844083b0cbb2f9c244738f
Author: Adam Grare <[email protected]>
Date:   Thu Oct 4 09:16:31 2018 -0400

    Merge pull request #73 from AlexanderZagaynov/BZ-1623862_http_proxy
    
    rename client's proxy option
    
    (cherry picked from commit ec11cad1b0d230581173c19f9111fb9613eb1dbe)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1623862

@simaishi
Copy link
Contributor

simaishi commented Oct 4, 2018

As per BZ Target Release, removing gaprindashvili/yes

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.

7 participants