Support operation `clear` on CloudObjectStoreContainer #14082

miha-plesko · 2017-02-27T16:19:34Z

Operation clear removes all CloudObjectStoreObjects from the CloudObjectStoreContainer. Since this kind of operation is not recognized by miq yet, I had to register it in supports_feature_mixin.rb.

miha-plesko · 2017-02-27T16:19:59Z

@miq-bot add_label enhancement,wip

miq-bot · 2017-03-02T15:07:16Z

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

miha-plesko · 2017-03-03T11:01:32Z

@durandom could you peek at this one as well? It's just a tiny upgrade of yesterday's PR that introduced *delete*_cloud_object_store_container only it's for *empty*_cloud_object_store_container.

durandom · 2017-03-03T11:28:55Z

@miha-plesko how does S3 implement this? Is there an empty operation?
I'm a bit torn between empty and empty_object_store_container. I think its fine for CRUD operations to have the bareword. But empty has no distinct meaning...

miha-plesko · 2017-03-03T12:34:00Z

@durandom aws-sdk names this operation "clear", see this line:
https://github.com/ManageIQ/manageiq-providers-amazon/pull/153/files#diff-26043fa5dbac3b44abc38abb3f0db938R9

I believe this operation could be reused on any model that groups other models. Like "empty orchestration stack" would mean to delete all instances within or "empty cloud network" would mean to remove all cloud subnets within.

I would go with empty_container to make is short. Would you insist on empty_object_store_container?

durandom · 2017-03-06T10:07:15Z

@blomquisg @agrare @bzwei @Ladas whats your opinion wrt naming this?
I think either empty for the short operation or empty_object_store_container to be explicit.
I tend towards the longer version

agrare · 2017-03-06T14:43:21Z

@durandom @miha-plesko I tend to like the more descriptive name as well but this is somewhat subjective, I don't get why you would have the long name for the main method and the short name for the raw method though...I would think they should keep the same name scheme.
Maybe :empty_cloud_object_store_container and :raw_empty_cloud_object_store_container?

Also for QUERYABLE_FEATURES I think :empty_container is way too generic, think it should match the method name so :empty_cloud_object_store_container?

miha-plesko · 2017-03-06T16:03:44Z

@durandom @agrare I've renamed to

cloud_object_store_container_empty
raw_cloud_object_store_container_empty

The reason I prefer suffix over prefix is miq_product_features.yml file where all the names are suffix-like:
https://github.com/ManageIQ/manageiq/pull/14082/files#diff-bccee92afbf0d3d195b7bcf343b1b6c4R622
Would this work for you?

(Should we decide to use prefix form empty_cloud_object_store_container, what would the product feature name be?)

blomquisg · 2017-03-06T18:56:18Z

app/models/cloud_object_store_container/operations.rb

+  end
+
+  def raw_cloud_object_store_container_empty
+    raise NotImplementedError, _("must be implemented in subclass")


It looks like the AWS API call is clear. I think that's more descriptive than empty.

My first impression with cloud_object_store_container_emtpy was that it was missing a question mark and it should be predicate method.

I think cloud_object_store_container_clear sounds like it's going to clear the container.

Or, do what @agrare said, and reverse the naming by putting the verb before the object: empty_cloud_object_store_container. In either case, I lean closer to sticking with clear instead of empty. But, I could live with either if the verb comes first.

My first impression with cloud_object_store_container_emtpy was that it was missing a question mark and it should be predicate method.

I thought the same thing when I saw :container_empty
I agree clear is more obvious especially if that is what the native method is called.

blomquisg

https://github.com/ManageIQ/manageiq/pull/14082/files#r104492419

miha-plesko · 2017-03-06T19:23:22Z

@blomquisg thanks for the review, I totally agree to cloud_object_store_container_clear. Fixed and repushed, please let me know if you have any other comments.

@miq-bot remove_label sql migration

durandom

generally ok

suffix vs. prefix: the supports feature mixin has more features with the verb being the prefix. But the product_features.yml has more suffixes, so...

durandom · 2017-03-06T21:38:55Z

app/models/mixins/supports_feature_mixin.rb

@@ -120,6 +120,7 @@ module SupportsFeatureMixin
    :update_security_group      => 'Security Group Update',
    :block_storage              => 'Block Storage',
    :object_storage             => 'Object Storage',
+    :cloud_object_store_container_clear => 'Clear Object Store Container',


could you put that in alphabetical order?

Oh, will try although there is no real alphabetical order to put in 😄

durandom · 2017-03-06T21:40:58Z

app/models/cloud_object_store_container.rb

@@ -15,4 +15,6 @@ class CloudObjectStoreContainer < ApplicationRecord
  alias_attribute :name, :key

  supports_not :delete, :reason => N_("Delete operation is not supported.")
+  supports_not :cloud_object_store_container_clear,
+               :reason => N_("Clear Object Store Container operation is not supported.")


you can omit the :reason and it will default to Operation not supported

miha-plesko

@durandom thanks for the review, I've applied your latest minor comments.

Regarding suffix vs prefix: the button on UI triggers whatever is written in miq_product_features.yml so if the function in supports_feature_mixin.rb is named differently, the UI controller needs to perform a mapping. My thinking was that we should avoid the mapping is possible.

miha-plesko · 2017-03-07T08:04:45Z

app/models/mixins/supports_feature_mixin.rb

@@ -120,6 +120,7 @@ module SupportsFeatureMixin
    :update_security_group      => 'Security Group Update',
    :block_storage              => 'Block Storage',
    :object_storage             => 'Object Storage',
+    :cloud_object_store_container_clear => 'Clear Object Store Container',


Oh, will try although there is no real alphabetical order to put in 😄

miha-plesko · 2017-03-07T08:05:00Z

app/models/cloud_object_store_container.rb

@@ -15,4 +15,6 @@ class CloudObjectStoreContainer < ApplicationRecord
  alias_attribute :name, :key

  supports_not :delete, :reason => N_("Delete operation is not supported.")
+  supports_not :cloud_object_store_container_clear,
+               :reason => N_("Clear Object Store Container operation is not supported.")


durandom · 2017-03-07T08:20:32Z

Regarding suffix vs prefix: the button on UI triggers whatever is written in miq_product_features.yml so if the function in supports_feature_mixin.rb is named differently, the UI controller needs to perform a mapping. My thinking was that we should avoid the mapping is possible.

Thats a great rationale 👍

miha-plesko · 2017-03-07T08:31:22Z

@blomquisg @agrare I like it how we're converging towards excelence here. Is there anything else to be improved, or did we converge? 🥇

Ladas · 2017-03-07T10:30:13Z

app/models/cloud_object_store_container.rb

@@ -15,4 +15,5 @@ class CloudObjectStoreContainer < ApplicationRecord
  alias_attribute :name, :key

  supports_not :delete, :reason => N_("Delete operation is not supported.")
+  supports_not :cloud_object_store_container_clear


We might need the :reason here, to get a user friendly message?

Nah, I just asked @miha-plesko to drop the reason. This is most likely not displayed in the UI at all and only adds a burden to i18n to translate messages that are not used.

Actually I'm inclined to remove all supports_not reason that are on a base class

ok, interesting

Ladas · 2017-03-07T10:40:53Z

@miha-plesko looks good in general.

Also, when you add a new feature, it's good to list it in some default user roles, https://github.com/Ladas/manageiq/blob/497be4eb4bf197e603eb81e3cd7c87d4ce1c97ed/db/fixtures/miq_user_roles.yml#L8-L8

Otherwise, superadmin will be the only one that sees it by default.

Operation `clear` removes all CloudObjectStoreObjects from the CloudObjectStoreContainer. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miq-bot · 2017-03-07T11:02:46Z

Checked commit xlab-si@92e7dfb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 👍

agrare · 2017-03-07T13:03:36Z

LGTM thanks @miha-plesko !

miq-bot added enhancement wip labels Feb 27, 2017

miha-plesko mentioned this pull request Feb 27, 2017

Support operation "clear" on CloudObjectStoreContainer ManageIQ/manageiq-providers-amazon#153

Merged

miq-bot added the sql migration label Feb 27, 2017

miha-plesko mentioned this pull request Feb 27, 2017

Support operation clear on CloudObjectStoreContainer ManageIQ/manageiq-ui-classic#498

Merged

chessbyte assigned Fryguy Feb 28, 2017

miha-plesko force-pushed the empty_aws_bucket branch from 566326d to f179dc7 Compare March 1, 2017 14:27

miha-plesko changed the title ~~[WIP] Enable operation "raw_empty" on CloudObjectStoreContainer~~ [WIP] Support operation "empty" on CloudObjectStoreContainer Mar 1, 2017

miha-plesko force-pushed the empty_aws_bucket branch from f179dc7 to 3d08efd Compare March 1, 2017 14:59

miq-bot added the unmergeable label Mar 2, 2017

miha-plesko force-pushed the empty_aws_bucket branch from 3d08efd to 50f2a9f Compare March 3, 2017 09:47

miha-plesko changed the title ~~[WIP] Support operation "empty" on CloudObjectStoreContainer~~ Support operation empty in CloudObjectStoreContainer Mar 3, 2017

miq-bot removed unmergeable wip labels Mar 3, 2017

miha-plesko force-pushed the empty_aws_bucket branch 2 times, most recently from 092691e to d3f99ae Compare March 3, 2017 14:35

miha-plesko changed the title ~~Support operation empty in CloudObjectStoreContainer~~ Support operation empty_container on CloudObjectStoreContainer Mar 3, 2017

miha-plesko closed this Mar 3, 2017

miha-plesko reopened this Mar 3, 2017

miha-plesko force-pushed the empty_aws_bucket branch from d3f99ae to 93ace7b Compare March 6, 2017 15:57

Fryguy assigned agrare and unassigned Fryguy Mar 6, 2017

blomquisg self-requested a review March 6, 2017 18:51

blomquisg reviewed Mar 6, 2017

View reviewed changes

blomquisg suggested changes Mar 6, 2017

View reviewed changes

miha-plesko force-pushed the empty_aws_bucket branch from 93ace7b to 35c76de Compare March 6, 2017 19:17

miha-plesko changed the title ~~Support operation empty_container on CloudObjectStoreContainer~~ Support operation clear on CloudObjectStoreContainer Mar 6, 2017

miq-bot removed the sql migration label Mar 6, 2017

durandom approved these changes Mar 6, 2017

View reviewed changes

miha-plesko force-pushed the empty_aws_bucket branch from 35c76de to 9f8449a Compare March 7, 2017 08:06

miha-plesko commented Mar 7, 2017

View reviewed changes

Ladas reviewed Mar 7, 2017

View reviewed changes

Support clear operation in CloudObjectStoreContainer

92e7dfb

Operation `clear` removes all CloudObjectStoreObjects from the CloudObjectStoreContainer. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miha-plesko force-pushed the empty_aws_bucket branch from 9f8449a to 92e7dfb Compare March 7, 2017 11:01

agrare merged commit 6d90d8d into ManageIQ:master Mar 7, 2017

agrare added the providers label Mar 7, 2017

chessbyte added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 17, 2017

tadeboro deleted the empty_aws_bucket branch March 30, 2018 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support operation `clear` on CloudObjectStoreContainer #14082

Support operation `clear` on CloudObjectStoreContainer #14082

miha-plesko commented Feb 27, 2017 •

edited

Loading

miha-plesko commented Feb 27, 2017

miq-bot commented Mar 2, 2017

miha-plesko commented Mar 3, 2017

durandom commented Mar 3, 2017

miha-plesko commented Mar 3, 2017 •

edited

Loading

durandom commented Mar 6, 2017

agrare commented Mar 6, 2017

miha-plesko commented Mar 6, 2017

blomquisg Mar 6, 2017

agrare Mar 6, 2017

blomquisg left a comment

miha-plesko commented Mar 6, 2017

durandom left a comment

durandom Mar 6, 2017

miha-plesko Mar 7, 2017

durandom Mar 6, 2017

miha-plesko Mar 7, 2017

miha-plesko left a comment

miha-plesko Mar 7, 2017

miha-plesko Mar 7, 2017

durandom commented Mar 7, 2017

miha-plesko commented Mar 7, 2017

Ladas Mar 7, 2017

durandom Mar 7, 2017

Ladas Mar 7, 2017

Ladas commented Mar 7, 2017

miq-bot commented Mar 7, 2017

agrare commented Mar 7, 2017

Support operation clear on CloudObjectStoreContainer #14082

Support operation clear on CloudObjectStoreContainer #14082

Conversation

miha-plesko commented Feb 27, 2017 • edited Loading

miha-plesko commented Feb 27, 2017

miq-bot commented Mar 2, 2017

miha-plesko commented Mar 3, 2017

durandom commented Mar 3, 2017

miha-plesko commented Mar 3, 2017 • edited Loading

durandom commented Mar 6, 2017

agrare commented Mar 6, 2017

miha-plesko commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blomquisg left a comment

Choose a reason for hiding this comment

miha-plesko commented Mar 6, 2017

durandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Mar 7, 2017

miha-plesko commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas commented Mar 7, 2017

miq-bot commented Mar 7, 2017

agrare commented Mar 7, 2017

Support operation `clear` on CloudObjectStoreContainer #14082

Support operation `clear` on CloudObjectStoreContainer #14082

miha-plesko commented Feb 27, 2017 •

edited

Loading

miha-plesko commented Mar 3, 2017 •

edited

Loading