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 Amazon block storage manager #107

Merged
merged 3 commits into from
Jan 18, 2017

Conversation

gberginc
Copy link
Contributor

Based on this comment from @roliveri, this PR replaces the current BlockStorageManager with StorageManager::EBS. This has two benefits. First, it explicitly names the block storage type, which is Elastic Block Store, and second, it allows for different types of block storage managers to be used by the amazon provider.

Changes were made in the provider itself as well as the inventory targets.

I am also suggesting changes in the cloud manager, i.e. replacing block_storage_manager with ebs_storage_manager. Please check whether these changes also make sense.

@Ladas @roliveri: please review. As soon as we agree on the structure, I will provide changes for the manageiq repository (automate and ext management system).

Before merging, we should sync to prevent failures in the main repo that occurred with #101 today (for which I apologise).

@miq-bot add_label euwe/no

@gberginc
Copy link
Contributor Author

@miq-bot assign @Ladas, @roliveri

@miq-bot
Copy link
Member

miq-bot commented Jan 17, 2017

@gberginc @Ladas, @roliveri is an invalid assignee, ignoring...

Initial implementation of the block storage manager presumed that Amazon
provider would only have a single block storage manager and consequently
used the generic class name `Amazon::BlockStorageManager`. @rolivery
suggested to change the name to indicate the actual type of the manager,
which in this case is EBS.

This patch thus replaces all occurrences where `BlockStorageManager` was
used before with `StorageManager::Ebs`.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
Since we are now explicitly adding EBS provider, this patch changes the
name of the corresponding dependent block storage manager.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
@gberginc
Copy link
Contributor Author

@roliveri Although you proposed StorageManager::EBS I changed it to StorageManager::Ebs. While updating automation models I realised I don't know how to actually make it look for EBS as it automatically expected Ebs in the name.

If there is a solution to this, I can revert to EBS.

Related PRs in the manageiq repository:

Following the renaming of the EBS storage manager, this patch also
changes the EBS storage key in the `config/settings.yml` file.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2017

Checked commits gberginc/manageiq-providers-amazon@99efe4b~...7cf0783 with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
13 files checked, 15 offenses detected

app/models/manageiq/providers/amazon/inventory/targets/storage_manager/ebs.rb

app/models/manageiq/providers/amazon/storage_manager/ebs/refresh_parser.rb

app/models/manageiq/providers/amazon/storage_manager/ebs/refresh_parser_inventory_object.rb

app/models/manageiq/providers/amazon/storage_manager/ebs/refresher.rb

spec/models/manageiq/providers/amazon/storage_manager/ebs/stubbed_refresher_spec.rb

@roliveri
Copy link
Member

@gberginc @Ladas This looks good to me.
Should I wait for this to be merged before merging ManageIQ/manageiq#13457 - to prevent interim test breakage?

@gberginc
Copy link
Contributor Author

@roliveri I have removed WIP status of the other PRs on manageiq repo (listed below). Before merging this one, I would like to get LGTMs on the other PRs.

I think the order of merges should be:

@roliveri
Copy link
Member

@gberginc @Ladas OK. I don't have merge permissions for this repo. So, once it's merged, I'll handle the others in the appropriate order.

@Ladas
Copy link
Contributor

Ladas commented Jan 18, 2017

@gberginc nice rename :-)

@Ladas Ladas merged commit eca0544 into ManageIQ:master Jan 18, 2017
@Ladas
Copy link
Contributor

Ladas commented Jan 18, 2017

@roliveri merged, you can proceed :-)

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.

4 participants