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

Flavors create and add methods #15552

Merged
merged 4 commits into from
Sep 6, 2017
Merged

Flavors create and add methods #15552

merged 4 commits into from
Sep 6, 2017

Conversation

alexander-demicev
Copy link

Added interface methods for creating and deleting flavors and made small
refactoring of existing method. 0penstack part will be implemented in
manageiq-providers-openstack.

@miq-bot miq-bot added the wip label Jul 12, 2017
@alexander-demicev alexander-demicev force-pushed the flavors branch 3 times, most recently from 2380b4e to 706cb65 Compare July 18, 2017 09:11
@aufi aufi mentioned this pull request Jul 18, 2017
Merged
petrblaho
petrblaho reviewed Jul 25, 2017
View reviewed changes
app/models/flavor.rb Outdated
:method_name => 'delete_flavor',
:role => 'ems operations',
:zone => ext_management_system.my_zone,
:args => [ext_management_system.id, options]
Copy link

Choose a reason for hiding this comment

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

options variable should not be here as it is not defined in this scope.
[ext_management_system.id] works well.

petrblaho
petrblaho suggested changes Jul 25, 2017
View reviewed changes
Copy link

@petrblaho petrblaho left a comment

Choose a reason for hiding this comment

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

Good work.

One place needs to be changed - that place with redundant options variable.

Other than that I like it - it works, it looks clean.

Thank you.

petrblaho
petrblaho reviewed Jul 26, 2017
View reviewed changes
app/models/flavor.rb Outdated
@@ -1,6 +1,8 @@
class Flavor < ApplicationRecord
include NewWithTypeStiMixin
include CloudTenancyMixin
include AvailabilityMixin
Copy link

Choose a reason for hiding this comment

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

First comment in AvailabilityMixin says # PLEASE PREFER supports_feature_mixin.rb OVER THIS.

Is there reason why we should use AvailabilityMixin instead of recommended SupportsFeatureMixin from https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/supports_feature_mixin.rb ?

Maybe I am missing something...

petrblaho
petrblaho reviewed Jul 27, 2017
View reviewed changes
Copy link

@petrblaho petrblaho left a comment

Choose a reason for hiding this comment

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

I like the code.

I suggest squashing fix commits into previous two.

petrblaho
petrblaho approved these changes Jul 31, 2017
View reviewed changes
Copy link

@petrblaho petrblaho left a comment

Choose a reason for hiding this comment

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

Ack from me. See how @blomquisg see that :-)

@alexander-demicev alexander-demicev changed the title [WIP] Flavors create and add methods Flavors create and add methods Aug 7, 2017
@miq-bot miq-bot removed the wip label Aug 7, 2017
@alexander-demicev alexander-demicev force-pushed the flavors branch 5 times, most recently from de2ae81 to d9925d3 Compare August 15, 2017 12:17
@alexander-demicev alexander-demicev mentioned this pull request Aug 15, 2017
Merged
@himdel himdel mentioned this pull request Aug 17, 2017
Merged
aufi
aufi reviewed Aug 17, 2017
View reviewed changes
app/models/flavor.rb
@@ -1,6 +1,9 @@
class Flavor < ApplicationRecord
include NewWithTypeStiMixin
include CloudTenancyMixin
include SupportsFeatureMixin

supports :create
Copy link
Member

Choose a reason for hiding this comment

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

@alexander-demichev Thanks for the PR!

Just curious, don't we need also supports :delete?

Copy link
Author

Choose a reason for hiding this comment

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

@aufi yeah, it`s there now

@miq-bot
Copy link
Member

miq-bot commented Aug 19, 2017

Checked commits alexander-demicev/manageiq@4e8cf39~...7521c70 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

aufi
aufi approved these changes Aug 21, 2017
View reviewed changes
Copy link
Member

@aufi aufi left a comment

Choose a reason for hiding this comment

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

Looks good to me, but needs core reviewer to merge. Maybe @blomquisg :-)

@alexander-demicev
Copy link
Author

alexander-demicev commented Sep 1, 2017 •
edited
Loading

@blomquisg Can you merge? :-)

@blomquisg blomquisg merged commit d65d0a0 into ManageIQ:master Sep 6, 2017
@blomquisg blomquisg added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 6, 2017
@agrare agrare mentioned this pull request Jul 6, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aufi aufi aufi approved these changes

@petrblaho petrblaho petrblaho approved these changes

Assignees

@blomquisg blomquisg

Labels
enhancement providers/cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexander-demicev @miq-bot @petrblaho @aufi @blomquisg @chessbyte