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

Allow restoring backups to new volumes #4623

Merged

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Sep 6, 2018

Allows choosing to restore a volume backup to a newly created volume, and adds a field for optionally naming that volume.

Partially implements https://bugzilla.redhat.com/show_bug.cgi?id=1514970
Depends on core and openstack provider PRs, as well as a new fog-openstack release.

restore_volume

@miq-bot
Copy link
Member

miq-bot commented Oct 2, 2018

This pull request is not mergeable. Please rebase and repush.

@mansam mansam force-pushed the allow-restoring-backups-to-new-volumes branch from 7ddbe7f to e250f6e Compare November 29, 2018 20:05
@miq-bot
Copy link
Member

miq-bot commented Nov 29, 2018

Checked commit mansam@e250f6e 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. 🏆

@mansam mansam changed the title [WIP] Allow restoring backups to new volumes Allow restoring backups to new volumes Nov 29, 2018
@mansam mansam changed the title Allow restoring backups to new volumes [WIP] Allow restoring backups to new volumes Nov 29, 2018
@mansam
Copy link
Contributor Author

mansam commented Nov 29, 2018

Depends on ManageIQ/manageiq#17952

@mansam mansam changed the title [WIP] Allow restoring backups to new volumes Allow restoring backups to new volumes Jan 17, 2019
@miq-bot miq-bot removed the wip label Jan 17, 2019
@martinpovolny
Copy link
Member

Hammer?

@mansam
Copy link
Contributor Author

mansam commented Jan 28, 2019

@martinpovolny No, this doesn't need to be backported to hammer.

@mansam
Copy link
Contributor Author

mansam commented Mar 7, 2019

@martinpovolny Do you know who might have time to review this?

@mansam
Copy link
Contributor Author

mansam commented Apr 3, 2019

Ping @martinpovolny, could you assign this to someone for review when you have the chance? It's the last piece of an RFE and the other pieces have long since been merged.

@ZitaNemeckova
Copy link
Contributor

No, this doesn't need to be backported to hammer.

@miq-bot add_label hammer/no

@ManSan I'm not sure about few backend things. Below is a form for a new Volume (please correct me if it isn't the correct one) and few things seems to be missing for a new Volume like a Cloud Tenant and Size in your form. Shouldn't Name be required?

Screen Shot 2019-04-12 at 3 48 00 PM

If Name is optional for a new Cloud in your form and Cloud Tenant and Size aren't needed then it is good :)

@mansam
Copy link
Contributor Author

mansam commented Apr 12, 2019

Hi @ZitaNemeckova, thanks for the review! Volume name is optional when restoring Openstack backups, size is unnecessary, and Cloud Tenant is automatically determined by the backend based on the backup being restored.

@ZitaNemeckova
Copy link
Contributor

@mansam thanks for explaining :)

LGTM 👍

@himdel himdel self-assigned this Apr 15, 2019
@himdel himdel closed this Apr 15, 2019
@himdel himdel reopened this Apr 15, 2019
@himdel
Copy link
Contributor

himdel commented Apr 15, 2019

Restarting travis, just to make sure nothing got broken since November, will merge when green :)

@himdel
Copy link
Contributor

himdel commented Apr 15, 2019

@mansam something did..

  1) AuthKeyPairCloudController#report_data when tile mode is selected returns key pairs with quadicons
     Failure/Error: expect(results['data']['rows'][0]['quad']).to have_key('fonticon')
       expected  to respond to `has_key?`
     # ./spec/controllers/auth_key_pair_cloud_controller_spec.rb:98

looks like quad is nil now.

@himdel
Copy link
Contributor

himdel commented Apr 15, 2019

Caused by
ff6d639, #3789 - removed CloudManager::AuthKeyPair decorator because the parent AuthPrivateKey was identical
together with
ManageIQ/manageiq@321d9d386fb, ManageIQ/manageiq#18633 - changed the parent of CloudManager::AuthKeyPair from AuthPrivateKey to Authentication.

So, looks like you'll need a https://github.com/ManageIQ/manageiq-decorators PR to add a decorator for Authentication or CloudManager::AuthKeyPair.

(cc @skateman)

@himdel
Copy link
Contributor

himdel commented Apr 15, 2019

Eeeh... never mind @mansam ..

I'm looking for a cause in this PR, finding none, but our master is red.

So... you don't have to do anything, we do :).

And merging, because except for the one failure shared with master, travis is indeed green.

@himdel himdel merged commit 0a1cd9b into ManageIQ:master Apr 15, 2019
@himdel himdel added this to the Sprint 109 Ending Apr 15, 2019 milestone Apr 15, 2019
@himdel
Copy link
Contributor

himdel commented Apr 15, 2019

@mansam
Copy link
Contributor Author

mansam commented Apr 15, 2019

@himdel @ZitaNemeckova thanks!

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