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

Fog google upgrade (to 1.3.3) #54

Merged
merged 20 commits into from
May 31, 2018
Merged

Conversation

tumido
Copy link
Member

@tumido tumido commented Apr 6, 2018

Upgrade fog-google to the latest release (1.3.3).

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1511099
Depends on: ManageIQ/manageiq#17258
Depends on: ManageIQ/azure-armrest#366

Most of the tasks mentioned above have rather a checklist character. It doesn't necessary mean everything has to change. I just need to go through it and correct any misbehavior...

@tumido
Copy link
Member Author

tumido commented Apr 6, 2018

@miq-bot add_label wip

@tumido
Copy link
Member Author

tumido commented Apr 6, 2018

@miq-bot add_label pending core

@tumido tumido force-pushed the fog_google_upgrade branch 4 times, most recently from 8f79084 to 3d3e706 Compare April 12, 2018 14:29
@tumido
Copy link
Member Author

tumido commented Apr 12, 2018

Once someone is reviewing this, please look closely on the event collection and parsing. I've decided to bypass the fog-google there, since it butchers the events when it tries to decode the event data from Base64. Despite the documentation says the events should be encoded in Base64, in reality for me they are showing as a plaintext. The API wasn't changed on Google side, so I don't understand what's happening. What also confuses me - we used to parse the events as StructPayload, now I see only JsonPayload.

In the end this is a working solution, which removes one dependency on fog-google and calls the google-api-client directly.

Maybe I'm just getting different events, I don't know...

@tumido tumido force-pushed the fog_google_upgrade branch 2 times, most recently from d9a02fa to 2b36faa Compare May 11, 2018 15:25
@tumido tumido force-pushed the fog_google_upgrade branch 3 times, most recently from 957348b to 32b978f Compare May 25, 2018 07:58
@tumido
Copy link
Member Author

tumido commented May 25, 2018

@miq-bot remove_label wip
All is done, ready for review! 🎉 🎆

@tumido tumido changed the title [WIP] Fog google upgrade (to 1.3.3) Fog google upgrade (to 1.3.3) May 25, 2018
@miq-bot miq-bot removed the wip label May 25, 2018
@agrare
Copy link
Member

agrare commented May 31, 2018

Rest of the comments can be addressed in a follow-up PR, LGTM @bronaghs

@bronaghs bronaghs merged commit ec8b909 into ManageIQ:master May 31, 2018
@bronaghs bronaghs added this to the Sprint 87 Ending Jun 4, 2018 milestone May 31, 2018
@agrare
Copy link
Member

agrare commented May 31, 2018

🎉

@bronaghs bronaghs added fine/yes and removed fine/no labels Jun 1, 2018
agrare added a commit that referenced this pull request Jun 4, 2018
Follow up on #54 "Fog google upgrade (to 1.3.3)" code cleanup
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 776f7c16e7ba878fbc63a6a122aeb5a6a0c0f1f9
Author: Bronagh Sorota <[email protected]>
Date:   Thu May 31 14:54:04 2018 -0400

    Merge pull request #54 from tumido/fog_google_upgrade
    
    Fog google upgrade (to 1.3.3)
    (cherry picked from commit ec8b909962ba9f6e6d9954fa1eff64c3fc7e3405)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1585821

@simaishi
Copy link
Contributor

@tumido ManageIQ/manageiq#17258 is listed as dependency, but that's not marked as fine/yes. I assume that's also needed? All other checklist have no PRs associated, so maybe nothing to do for Fine branch there... but not sure. Fine branch already uses azure-armrest which includes ManageIQ/azure-armrest#366, so that's taken care of.

Also this PR conflicts on cherry-pick. Please create a separate PR for Fine branch.

@tumido
Copy link
Member Author

tumido commented Aug 27, 2018

@simaishi can you please guide me how to do that? It seems there's no fine branch in https://github.com/ManageIQ/manageiq-providers-google

@simaishi
Copy link
Contributor

@tumido Please create a PR in manageiq repo.

@simaishi
Copy link
Contributor

Backported to Fine via ManageIQ/manageiq#17914

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