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

Fix the dynamic service task naming generation for subclasses #18511

Merged
merged 1 commit into from
Mar 4, 2019

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Mar 4, 2019

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1684092

The service task magic generation should've used safe_constantize.

for subclasses like ServiceOrchestration we need the base class name; for Vms we just need the class name

This fixes the tests, too, cause they should've used add_resource.

@d-m-u
Copy link
Contributor Author

d-m-u commented Mar 4, 2019

@miq-bot add_label bug
@miq-bot add_reviewer @bdunne

@miq-bot miq-bot added the bug label Mar 4, 2019
@miq-bot miq-bot requested a review from bdunne March 4, 2019 13:36
@d-m-u d-m-u changed the title Fix the dynamic task naming generation for subclasses Fix the dynamic service task naming generation for subclasses Mar 4, 2019
@d-m-u d-m-u force-pushed the fixing_service_task_type branch from cc245ab to 077dea1 Compare March 4, 2019 13:39
@d-m-u
Copy link
Contributor Author

d-m-u commented Mar 4, 2019

@miq-bot add_label hammer/yes

app/models/service_retire_task.rb Outdated Show resolved Hide resolved
expect(ServiceRetireTask.count).to eq(1)
end
expect(service_retire_task.description).to eq("Service Retire for: #{service.name} - ")
expect(ServiceRetireTask.count).to eq(2)
Copy link
Member

Choose a reason for hiding this comment

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

Why 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the top level service has one too.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, so one for the Service and one for the ServiceOrchestration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

for subclasses like ServiceOrchestration we need the base class name
for Vms we just need the class name
@d-m-u d-m-u force-pushed the fixing_service_task_type branch from 077dea1 to 98950cc Compare March 4, 2019 15:47
@miq-bot
Copy link
Member

miq-bot commented Mar 4, 2019

Checked commit d-m-u@98950cc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@d-m-u
Copy link
Contributor Author

d-m-u commented Mar 4, 2019

@tinaafitz please review, it's for that bug last week about retirement

@bdunne bdunne merged commit 7870b3f into ManageIQ:master Mar 4, 2019
@bdunne bdunne self-assigned this Mar 4, 2019
@bdunne bdunne added this to the Sprint 106 Ending Mar 4, 2019 milestone Mar 4, 2019
simaishi pushed a commit that referenced this pull request Mar 6, 2019
Fix the dynamic service task naming generation for subclasses

(cherry picked from commit 7870b3f)

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

simaishi commented Mar 6, 2019

Hammer backport details:

$ git log -1
commit 71f3586d315408f9d8dd30d9c44555a8acdc0b78
Author: Brandon Dunne <[email protected]>
Date:   Mon Mar 4 11:27:26 2019 -0500

    Merge pull request #18511 from d-m-u/fixing_service_task_type
    
    Fix the dynamic service task naming generation for subclasses
    
    (cherry picked from commit 7870b3faeb1c649f79a7100b3a7b1007b2327cfa)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1685737

@d-m-u d-m-u deleted the fixing_service_task_type branch September 26, 2019 10:34
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.

4 participants