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.

Sign up for GitHub

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

Jump to bottom

rename storages to ceph #71

Merged
merged 1 commit into from
Sep 29, 2016
Merged

rename storages to ceph #71

merged 1 commit into from
Sep 29, 2016

Conversation

MaximilianMeister
Copy link
Member

the barclamp name fits better than the abstracted name

houndci-bot
houndci-bot reviewed Sep 15, 2016
View reviewed changes
crowbar_framework/app/controllers/api/ceph_controller.rb
@@ -14,23 +14,22 @@
# limitations under the License.
#

class Api::StoragesController < ApiController
class Api::CephController < ApiController
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with Hound here. I've seen problems in the past caused by not following this guideline.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to cleanup card https://trello.com/c/8p1DvJQ2/1931-cleanup-upgrade-related-api as we have to change it in multiple places (in other files/repos as well)

@MaximilianMeister MaximilianMeister mentioned this pull request Sep 15, 2016
Merged
jsuchome
jsuchome reviewed Sep 15, 2016
View reviewed changes
crowbar_framework/app/controllers/api/ceph_controller.rb
before_action :set_ceph

api :GET, "/api/storages", "List all Ceph storages"
api :GET, "/api/ceph", "Show the Ceph object"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I do not like the "object", it looks like something inside ceph storage. What about "configuration"? (Not much better...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that something less vague would be better. What does the API call return, exactly? Ideally the description should clearly reflect that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now nothing, but it should display some general data (see also GET /api/crowbar). what about returning the ceph version for a start?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw that we don't store the ceph version anywhere as far as i could see. but lets just drop this endpoint for now as we don't really have a use case

aspiers
aspiers requested changes Sep 15, 2016
View reviewed changes
Copy link
Member

@aspiers aspiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM (I think - my Rails routes knowledge is a bit out of date) - just one or two minor nitpicks.

crowbar_framework/app/controllers/api/ceph_controller.rb
@@ -14,23 +14,22 @@
# limitations under the License.
#

class Api::StoragesController < ApiController
class Api::CephController < ApiController
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with Hound here. I've seen problems in the past caused by not following this guideline.

crowbar_framework/app/controllers/api/ceph_controller.rb
before_action :set_ceph

api :GET, "/api/storages", "List all Ceph storages"
api :GET, "/api/ceph", "Show the Ceph object"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that something less vague would be better. What does the API call return, exactly? Ideally the description should clearly reflect that.

@MaximilianMeister MaximilianMeister force-pushed the rename-api branch 2 times, most recently from 826c414 to 47ebfc0 Compare September 16, 2016 07:46
@MaximilianMeister
Copy link
Member Author

@aspiers @jsuchome updated (not sure though why the comments are duplicated, and the hound doesn't disappear)

@jsuchome
Copy link
Member

+1

@aspiers
Copy link
Member

aspiers commented Sep 21, 2016

@MaximilianMeister It seems that github's new features still have some bugs to iron out ;-)

aspiers
aspiers reviewed Sep 21, 2016
View reviewed changes
Copy link
Member

@aspiers aspiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rename looks fine but I'm a bit puzzled about the design of the code which already existed before this PR.

crowbar_framework/app/models/api/ceph.rb
@@ -15,7 +15,7 @@
#

module Api
class Storage < Tableless
class Ceph < Tableless
def repocheck
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this an instance method rather than a class method?

crowbar_framework/app/controllers/api/ceph_controller.rb
protected

def set_ceph
@ceph = Api::Ceph.new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this PR isn't introducing or even really changing this, but I don't understand why it's an instance method or why an instance variable is being assigned, since I can't see any instance-specific state. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good that you mention that, I already thought about this as well. I added a task to the cleanup trellocard

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aspiers see #72

@aspiers aspiers removed their assignment Sep 21, 2016
@MaximilianMeister MaximilianMeister mentioned this pull request Sep 22, 2016
Merged
@MaximilianMeister MaximilianMeister force-pushed the rename-api branch 2 times, most recently from b38ddb2 to 755e2ca Compare September 27, 2016 08:20
@MaximilianMeister
rename storages to ceph and drop unused endpoints
3f9527c 
the barclamp name fits better than the abstracted name
@MaximilianMeister
Copy link
Member Author

rebased and added an example to the api doc (according to crowbar/crowbar-core#682)

jsuchome
jsuchome approved these changes Sep 27, 2016
View reviewed changes
Copy link
Member

@jsuchome jsuchome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

rsalevsky
rsalevsky requested changes Sep 27, 2016
View reviewed changes
Copy link
Member

@rsalevsky rsalevsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you splitt this change into two separate PRs? This are two totally different topics.

@MaximilianMeister
Copy link
Member Author

Can you splitt this change into two separate PRs

dropped the commit

@MaximilianMeister
Copy link
Member Author

@aspiers ping

@aspiers aspiers merged commit 969420a into crowbar:master Sep 29, 2016
@MaximilianMeister MaximilianMeister deleted the rename-api branch September 29, 2016 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@rsalevsky rsalevsky rsalevsky approved these changes

@dirkmueller dirkmueller dirkmueller approved these changes

@jsuchome jsuchome jsuchome approved these changes

@aspiers aspiers aspiers approved these changes

Assignees

@jsuchome jsuchome

@rsalevsky rsalevsky

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MaximilianMeister @jsuchome @aspiers @dirkmueller @rsalevsky @houndci-bot