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

Added before_save to MiqTask to initialize MiqTask#started_on when task become active #16863

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Jan 22, 2018

Before:
MiqTask#started_on was not set if state attribute updated directly (like task.update_attributes(:state => MiqTask::STATE_ACTIVE))
related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1536126

After:
Added before_save callback to initialize #started_on when state set to "Active"

@miq-bot add-label core, technical debt

\cc @gtanzillo @jameswnl

@agrare
Copy link
Member

agrare commented Jan 22, 2018

@yrudman so this will set started_on to be essentially equal to created_on which isn't very useful.
Could the UI show created_on like it used to and we can work on setting a useful started_on when the first queue item in the task is delivered (future release)?

@@ -27,7 +27,8 @@ class MiqTask < ApplicationRecord
before_validation :initialize_attributes, :on => :create

before_destroy :check_active, :check_associations

before_save :started
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps ensure_started as a method name? started is an adjective, and a verb reads nicer.

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM... Just change the method name.

@yrudman
Copy link
Contributor Author

yrudman commented Jan 22, 2018

@agrare created_on populated when record created,
started_on would be populated only when task become active (delivered by queue worker and recipient will update state attribute to "Active").

We do show data previously displayed in Started column, only column re-named to Queued

@agrare
Copy link
Member

agrare commented Jan 22, 2018

@yrudman 👍 I missed the if state == STATE_ACTIVE part and thought you were setting started_on on first save

@yrudman
Copy link
Contributor Author

yrudman commented Jan 22, 2018

@miq-bot add-label gaprindashvili/yes

@miq-bot
Copy link
Member

miq-bot commented Jan 22, 2018

Checked commits yrudman/manageiq@a56d3b7~...0c8969b with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@yrudman
Copy link
Contributor Author

yrudman commented Jan 22, 2018

@Fryguy method name changed, ready for merge

@Fryguy Fryguy added bug and removed technical debt labels Jan 22, 2018
@Fryguy Fryguy merged commit d992f98 into ManageIQ:master Jan 22, 2018
@Fryguy Fryguy added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 22, 2018
@Fryguy Fryguy added the blocker label Jan 22, 2018
simaishi pushed a commit that referenced this pull request Jan 23, 2018
…to-initialize-strted_on

Added before_save to MiqTask to initialize MiqTask#started_on when task become active
(cherry picked from commit d992f98)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 4e25d98d2fcc0a6222ee484b401d846010a1b6e3
Author: Jason Frey <[email protected]>
Date:   Mon Jan 22 16:08:40 2018 -0500

    Merge pull request #16863 from yrudman/added-before-safe-to-miq-task-to-initialize-strted_on
    
    Added before_save to MiqTask to initialize MiqTask#started_on when task become active
    (cherry picked from commit d992f980a857db789c8aa8b3636bf1b6559ca539)

@yrudman yrudman deleted the added-before-safe-to-miq-task-to-initialize-strted_on branch January 23, 2018 19:08
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