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

ProcessTasksMixinin.invoke_tasks_remote should not send AuditEvent(:success) if invocation failed #18565

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Mar 18, 2019

ISSUE: ProcessTasksMixinin.invoke_tasks_remotealways sending single AuditEvent(:success) . So, in case all operations failed - AuditEvent(:success) would be send anyway

FIX: send appropriate AuditEvent for each remote resource

@miq-bot add-label bug. core

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

@miq-bot
Copy link
Member

miq-bot commented Mar 18, 2019

@yrudman Cannot apply the following label because they are not recognized: bug. core

@miq-bot miq-bot added the wip label Mar 18, 2019
@yrudman yrudman force-pushed the invoke_task_remote-should-not-send-success-if-invocation-failed branch 7 times, most recently from 41a17eb to 953233a Compare March 20, 2019 21:45
@yrudman yrudman changed the title [WIP] Invoke task remote should not send 'success' AuditEvent if invocation failed Invoke task remote should not send 'success' AuditEvent if invocation failed Mar 20, 2019
@miq-bot miq-bot removed the wip label Mar 20, 2019
@yrudman yrudman changed the title Invoke task remote should not send 'success' AuditEvent if invocation failed ProcessTasksMixinin.invoke_tasks_remote should not send AuditEvent(:success) if invocation failed Mar 21, 2019
@yrudman
Copy link
Contributor Author

yrudman commented Mar 21, 2019

@carbonin could you review

@yrudman yrudman force-pushed the invoke_task_remote-should-not-send-success-if-invocation-failed branch from 953233a to ed5d8c0 Compare March 21, 2019 18:40
@yrudman yrudman force-pushed the invoke_task_remote-should-not-send-success-if-invocation-failed branch 3 times, most recently from 31b0c5a to 6227567 Compare March 27, 2019 13:23
Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

Just the one comment. Looks good otherwise.

app/models/mixins/process_tasks_mixin.rb Outdated Show resolved Hide resolved
yrudman added 3 commits March 27, 2019 14:30
…e to it found on global region, after that object got deleted on remote region but global region attempting to use previously found reference for Centralized Administration. What happened is: attempt to execute queue item raising 'resource not found' and message requeud. Desired behavior: if resource not found then do not requeue message

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1689330
…separetly instead of one AuditEvent per region
@yrudman yrudman force-pushed the invoke_task_remote-should-not-send-success-if-invocation-failed branch from 6227567 to 65f48ce Compare March 27, 2019 18:41
@yrudman yrudman force-pushed the invoke_task_remote-should-not-send-success-if-invocation-failed branch from 65f48ce to 5c8ceb2 Compare March 27, 2019 18:43
@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2019

Checked commits yrudman/manageiq@220d37e~...5c8ceb2 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. 🍰

@JPrause
Copy link
Member

JPrause commented Mar 27, 2019

@yrudman if this will be able to be backported, can you add the hammer/yes label

@yrudman
Copy link
Contributor Author

yrudman commented Mar 27, 2019

@miq-bot add-label hammer/yes

@carbonin carbonin self-assigned this Mar 28, 2019
@carbonin carbonin merged commit 047ef4c into ManageIQ:master Mar 28, 2019
@carbonin carbonin added this to the Sprint 108 Ending Apr 1, 2019 milestone Mar 28, 2019
@yrudman yrudman deleted the invoke_task_remote-should-not-send-success-if-invocation-failed branch March 28, 2019 15:24
simaishi pushed a commit that referenced this pull request Mar 29, 2019
…send-success-if-invocation-failed

ProcessTasksMixinin.invoke_tasks_remote should not send AuditEvent(:success) if invocation failed

(cherry picked from commit 047ef4c)

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

Hammer backport details:

$ git log -1
commit d4d38a41bf646f213ce4b513ab7c268e33ba7f4b
Author: Nick Carboni <[email protected]>
Date:   Thu Mar 28 11:22:41 2019 -0400

    Merge pull request #18565 from yrudman/invoke_task_remote-should-not-send-success-if-invocation-failed
    
    ProcessTasksMixinin.invoke_tasks_remote should not send AuditEvent(:success) if invocation failed
    
    (cherry picked from commit 047ef4cbc0e564988e3a93de73d6f16e85e456fb)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1693817

@yrudman
Copy link
Contributor Author

yrudman commented Apr 24, 2019

@miq-bot add-label changelog/yes

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