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

Store an event when a task becomes enabled and executes #319

Merged
merged 2 commits into from Jun 21, 2021

Conversation

lornasong
Copy link
Member

When a task is disabled and then enabled via CLI, the task may execute if there
are changes. Whenever a task executes, an event should be stored.

When enable CLI was implemented, storing an event on task execution was
unintentionally not implemented. This change fixes this and stores an event.

Changes:

  • Identifies when a task is executed: when ?run=now i.e. runOp == driver.RunOptionNow
  • Stores an event
  • Retrieves an error that may result from driver.UpdateTask to store in the event

Fixes: #318

When a task is disabled and then enabled via CLI, the task may execute if there
are changes. Whenever a task executes, an event should be stored.

When enable CLI was implemented, storing an event on task execution was
unintentionally not implemented. This change fixes this and stores an event.

Changes:
 - Identifies when a task is executed: when ?run=now i.e. runOp == driver.RunOptionNow
 - Stores an event
 - Retrieves an error that may result from driver.UpdateTask to store in the event
@lornasong lornasong added the bug Something isn't working label Jun 18, 2021
@lornasong lornasong added this to the v0.2.0 milestone Jun 18, 2021
@lornasong lornasong requested a review from a team June 18, 2021 21:14
Copy link
Contributor

@findkim findkim left a comment

Choose a reason for hiding this comment

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

Good catch that the update API was missing the event tracking. Changes look good!

Comment on lines +137 to +140
defer func() {
ev.End(storedErr)
log.Printf("[TRACE] (api.task) adding event %s", ev.GoString())
if err := h.store.Add(*ev); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: The defer func seems to be initialized even if there is an error, where ev may be nil, then it would panic here. It's quite unlikely NewEvent will ever error, but can you update this where panics can't be a part of a possible path? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Thanks for raising this! Some above code checks for the edge-case that would cause NewEvent to return error i.e. when taskName is empty. But I'm realizing that this dependency isn't clear at all and that code may evolve in a way that opens up for more panic. I decided to add a return when there's an error creating an event. Thank you!

@lornasong lornasong merged commit be09439 into v0.2.0 Jun 21, 2021
@lornasong lornasong deleted the enable-event branch June 21, 2021 17:40
@lornasong lornasong linked an issue Jun 21, 2021 that may be closed by this pull request
@findkim findkim added the backport/0.1 Changes are backported to 0.1 label Jun 21, 2021
sarahethompson pushed a commit that referenced this pull request Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/0.1 Changes are backported to 0.1 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling a task executes the task but does not store an event
2 participants