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

move #my_zone from ArchivedMixin to OldEmsMixin #17539

Merged
merged 1 commit into from
Jun 6, 2018

Conversation

bzwei
Copy link
Contributor

@bzwei bzwei commented Jun 6, 2018

Not every model that needs ArchivedMixin has ext_management_system or #old_ext_management_sytem. For those do need to use #old_ext_management_system now they should include OldEmsMixin.

This is a followup PR of #17480
Fixes issue #17535

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1594873

Gap backport: #17540

Not every model that needs ArchivedMixin has old_ext_management_sytem. For those do need now
they should include OldEmsMixin.
@bzwei
Copy link
Contributor Author

bzwei commented Jun 6, 2018

@miq-bot assign @agrare
@miq-bot label bug

@miq-bot
Copy link
Member

miq-bot commented Jun 6, 2018

@bzwei unrecognized command 'label', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

@miq-bot
Copy link
Member

miq-bot commented Jun 6, 2018

Checked commit bzwei@8e753a1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 1 offense detected

app/models/mixins/old_ems_mixin.rb

@agrare agrare added the bug label Jun 6, 2018
@agrare
Copy link
Member

agrare commented Jun 6, 2018

Looks good to me, let me run the kubernetes and openshift specs with this applied to master and I'll merge if that's green

@bzwei
Copy link
Contributor Author

bzwei commented Jun 6, 2018

@miq-bot add_label Gaprindashvili/yes

@agrare
Copy link
Member

agrare commented Jun 6, 2018

Kubernetes and Openshift specs pass

@agrare agrare merged commit 8bd1664 into ManageIQ:master Jun 6, 2018
@agrare agrare added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 6, 2018
@bzwei bzwei deleted the old_ems_id branch June 6, 2018 20:19
@agrare
Copy link
Member

agrare commented Jun 25, 2018

simaishi pushed a commit that referenced this pull request Jun 28, 2018
move #my_zone from ArchivedMixin to OldEmsMixin
(cherry picked from commit 8bd1664)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1595418
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 17a3e84538a452304556990478627e1d080c7e7b
Author: Adam Grare <agrare@redhat.com>
Date:   Wed Jun 6 15:45:12 2018 -0400

    Merge pull request #17539 from bzwei/old_ems_id
    
    move #my_zone from ArchivedMixin to OldEmsMixin
    (cherry picked from commit 8bd16646c6efd64a808731f50d99eef12bc53962)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1595418

end

# Needed for metrics
def my_zone
Copy link
Member

Choose a reason for hiding this comment

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

Just saw this now, but this is a really weird place for this method. I get that OldEmsMixin should enhance the method, but it feels more correct for there to be a method in the base class that does my_zone normally, and this method can do what it does followed by calling super. @agrare ?

Copy link
Member

Choose a reason for hiding this comment

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

@Fryguy yeah I see what you mean, there doesn't seem to be a whole lot that the base model's my_zone method does though, they all look like:

  def my_zone
    ems = ext_management_system
    ems ? ems.my_zone : MiqServer.my_zone
  end

So we would have to refactor them all to allow this to get in the middle before falling back to MiqServer.my_zone.
Maybe a method my_ems which my_zone would call then this mixin could enhance the my_ems method?

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.

5 participants