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

Add Amazon S3 manager EMS #13557

Closed
wants to merge 1 commit into from
Closed

Conversation

aiperon
Copy link
Contributor

@aiperon aiperon commented Jan 18, 2017

Register S3 with the ext management system.
Similar to #13539.

Depends on ManageIQ/manageiq-providers-amazon#106.

Reqiured to CI tests pass.

@miq-bot add_label providers/amazon,providers/storage

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2017

Checked commit xlab-si@38ae9dc with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2017

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

@Fryguy
Copy link
Member

Fryguy commented Jan 18, 2017

Not sure I like the fact that the manager takes the toplevel s3 name...I would have expected ec2_object_storage . cc @roliveri . The corresponding PR in amazon was merged already by @Ladas but this one is now blocking ManageIQ from going green. Thoughts?

@gberginc gberginc mentioned this pull request Jan 19, 2017
@gberginc
Copy link
Contributor

@aiperon should be closed due to the combined PR related to the S3 changes #13580

@aiperon aiperon closed this Jan 19, 2017
@Ladas
Copy link
Contributor

Ladas commented Jan 19, 2017

@Fryguy I think they just follow OpenStack way, where they've renamed it to CinderManager and SwiftManager, according to the name of the service. So not sure what was the motivation really. :-)

But then it fits to call the AWS managers according to the name of the service, so we are consistent there.

I would prefer Provider::Object/BlockStorage, as it will be more understandable across providers. :-)

@aiperon
Copy link
Contributor Author

aiperon commented Jan 19, 2017

I think the only reason for ec2_object_storage is that StorageManager::S3 is a child provider of EC2 provider. Although, instead of EBS, S3 is not related to EC2 naturally.
Child provider Implementation was minimal change to getting things done. Otherwise, we had to define the way to add storage manager separately from cloud manager. At this moment there isn't full picture of how it should look's like.

@roliveri
Copy link
Member

@Fryguy _ We discussed this. Didn't we agree that concrete names are better, enabling the support different implementations of a given type of storage (Block, Object, etc).

So Provider::Object/BlockStorage, Provider::Object/BlockStorage and ec2_object_storage would be wrong. Even if the chances are less likely in the Amazon environment that they'll have multiple implementations, we should still be consistent with the other storage managers. In addition Amazon instances can access external storage subsystems that provide the same type of storage - block for example, and we'll have to differentiate between those managers as well.

@tadeboro tadeboro deleted the add_amazon_s3_ems branch March 30, 2018 07:56
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