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

Separate queue put from publish event so we don't queue put twice #18178

Merged
merged 1 commit into from
Nov 27, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Nov 8, 2018

The main repo part of https://github.com/ManageIQ/manageiq-api/pull/506/files. We're currently not creating custom button events for custom buttons with dialogs, because invoke never gets called. But invoke also does a queue put, which both custom buttons with and without dialogs handle, so the line to create custom button events needed to be later than the queue put as to not do the queue twice.

related

ManageIQ/manageiq-api#506

@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 8, 2018

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

@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 8, 2018

@miq-bot add_label hammer/yes, blocker

@miq-bot miq-bot added the bug label Nov 8, 2018
@miq-bot miq-bot requested a review from bdunne November 8, 2018 19:58
@d-m-u d-m-u changed the title Separate queue put from publish event so we don't queue put twice [WIP] Separate queue put from publish event so we don't queue put twice Nov 8, 2018
@miq-bot miq-bot added the wip label Nov 8, 2018
@d-m-u d-m-u closed this Nov 13, 2018
@d-m-u d-m-u reopened this Nov 13, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 13, 2018

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Separate queue put from publish event so we don't queue put twice Separate queue put from publish event so we don't queue put twice Nov 13, 2018
@miq-bot miq-bot removed the wip label Nov 13, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 26, 2018

@miq-bot add_reviewer @mkanoor
Madhu, the related one is here: ManageIQ/manageiq-api#506 and if you could please look at it too that'd be fantastic, thanks, cause Brandon's out this week

@miq-bot miq-bot requested a review from mkanoor November 26, 2018 13:41
@@ -100,7 +100,8 @@ def invoke(target, source = nil)
MiqQueue.put(queue_opts(target, args))
end

def publish_event(source, target, args)
def publish_event(source, target, args = nil)
args ||= resource_action.automate_queue_hash(target, {}, User.current_user)
Copy link
Member

Choose a reason for hiding this comment

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

Would be useful to test that the method calls resource_action.automate_queue_hash when called without an args parameter since that is the logic being added.

@d-m-u d-m-u force-pushed the main_bz_bz1511126 branch 2 times, most recently from bbea863 to c1fec3c Compare November 26, 2018 20:41
describe "publish event" do
context "with blank args" do
it "resource action calls automate_queue_hash" do
Timecop.freeze(Time.now.utc) do
Copy link
Member

Choose a reason for hiding this comment

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

Does not look like we need Timecop here?


context "with args" do
it "resource action doesn't call automate_queue_hash" do
Timecop.freeze(Time.now.utc) do
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

context "with args" do
it "resource action doesn't call automate_queue_hash" do
Timecop.freeze(Time.now.utc) do
expect(resource_action).not_to receive(:automate_queue_hash).with(vm, {}, user)
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the with(<params>) since you do not expect it to be called at all.

@d-m-u d-m-u force-pushed the main_bz_bz1511126 branch from c1fec3c to 6a6b595 Compare November 27, 2018 12:24
@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2018

Checked commit d-m-u@6a6b595 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. 👍

@gmcculloug gmcculloug merged commit 7b34f5b into ManageIQ:master Nov 27, 2018
@gmcculloug gmcculloug added this to the Sprint 100 Ending Dec 3, 2018 milestone Nov 27, 2018
simaishi pushed a commit that referenced this pull request Nov 27, 2018
Separate queue put from publish event so we don't queue put twice

(cherry picked from commit 7b34f5b)
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 1f11197a384ad3d69e74308e1fd06bddf92a906c
Author: Greg McCullough <[email protected]>
Date:   Tue Nov 27 08:41:13 2018 -0500

    Merge pull request #18178 from d-m-u/main_bz_bz1511126
    
    Separate queue put from publish event so we don't queue put twice
    
    (cherry picked from commit 7b34f5b229a168240ea26a5cc72b91d62d11d14e)

@d-m-u d-m-u deleted the main_bz_bz1511126 branch February 1, 2019 20:48
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