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

Return result of destroy action to user, not nil #14097

Merged
merged 2 commits into from
Mar 21, 2017

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Feb 28, 2017

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

The cause of the bug was, that delete_resource action must return action-result-hash and this was returning nil.

@miq-bot add_label bug, euwe/yes, api
@miq-bot assign @abellotti

@chessbyte
Copy link
Member

@isimluk Any way to write a test validating that destroy only gets called once? It would then fail before this PR and succeed with it.

@Ladas
Copy link
Contributor

Ladas commented Feb 28, 2017

+1

@isimluk I am glad I could help. :-)

although the hook seems to be conditional https://github.com/Ladas/manageiq/blob/cdc6c6319475c225cf0e5b970298c8a5a6bf8d29/app/models/orchestration_template_vnfd.rb#L6-L6 so it should not fire, unless you change the condition?

@isimluk
Copy link
Member Author

isimluk commented Mar 1, 2017

Good food for thoughts.

This will need to have some work on remote_proxy thing.

@miq-bot add_label wip

@isimluk isimluk changed the title Do not overide default action because of raw_destroy [WIP] Do not overide default action because of raw_destroy Mar 1, 2017
@miq-bot miq-bot added the wip label Mar 1, 2017
Fixex https://bugzilla.redhat.com/show_bug.cgi?id=1414881

The cause of the bug was, that delete_resource action must return
action-result-hash and the raw action was returning nil.
@isimluk
Copy link
Member Author

isimluk commented Mar 21, 2017

@abellotti, I have run into a dead end with #14266, so I changed this pr to just fix the bug. It is now ready for review. Thanks!

@isimluk isimluk removed the wip label Mar 21, 2017
@isimluk isimluk changed the title [WIP] Do not overide default action because of raw_destroy Return result of destroy action to user, not nil Mar 21, 2017
@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Some comments on commits isimluk/manageiq@96e621c~...5af77c5

spec/requests/api/orchestration_template_spec.rb

  • ⚠️ - 120 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Checked commits isimluk/manageiq@96e621c~...5af77c5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

@abellotti
Copy link
Member

That fix looks right. Is this euwe/yes still ? if so does it depend on other PRs and have those been euwe/yes. Thanks.

@Ladas
Copy link
Contributor

Ladas commented Mar 21, 2017

looks great 👍

@isimluk
Copy link
Member Author

isimluk commented Mar 21, 2017

Is this euwe/yes still ?

Yes.

if so does it depend on other PRs and have those been euwe/yes.

Unfortunately, no. Dust settles on the other parts of the code.

@abellotti
Copy link
Member

Thanks @isimluk 👍

@abellotti abellotti merged commit 9aa0cfe into ManageIQ:master Mar 21, 2017
@abellotti abellotti added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 21, 2017
simaishi pushed a commit that referenced this pull request Apr 13, 2017
Return result of destroy action to user, not nil
(cherry picked from commit 9aa0cfe)

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

Euwe backport details:

$ git log -1
commit 9608b9009e5306ede9b6862abba3eee681b02f83
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Tue Mar 21 15:03:23 2017 -0400

    Merge pull request #14097 from isimluk/rhbz#1414881
    
    Return result of destroy action to user, not nil
    (cherry picked from commit 9aa0cfe1ea89a277b7cf28efe0024ee21a0d3b3c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1434952

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.

6 participants