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

Task is cleaning default msg fix #842

Merged
merged 1 commit into from Jan 21, 2016

Conversation

Projects
None yet
3 participants
@Calvinp
Contributor

Calvinp commented Jan 14, 2016

Pull request to solve an issue where the message field for a bounced task would be blank.

Added a default message which will appear when a task is being cleaned up after a bounce

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Jan 14, 2016

Member

Diff will be better once expiring_Actions is merged in, quick link to diff for this branch
https://github.com/HubSpot/Singularity/compare/expiring_actions...task-is-cleaning-default-msg-fix?expand=1

Member

ssalinas commented Jan 14, 2016

Diff will be better once expiring_Actions is merged in, quick link to diff for this branch
https://github.com/HubSpot/Singularity/compare/expiring_actions...task-is-cleaning-default-msg-fix?expand=1

@ssalinas ssalinas added the hs_staging label Jan 15, 2016

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Jan 15, 2016

Member

Thanks for the PR, Calvin!

A few notes:

  • It looks like some old expiring_actions commits bled into this branch. Now that the branch has been merged to master, could you rebase?
  • I think the original issue was unclear, sorry. When the new message field was added, the Task is cleaning banner was updated to only display the message value (which wasn't ideal, and is causing the blank "Task is cleaning:" banner you saw). What we want to do instead is for the banner to display cleanup.cleanupType, and the message in ( )'s only if there's one set. (examples: Task is cleaining: BOUNCING and Task is cleaning: BOUNCING (my custom message here))
  • With the new display mentioned above, it's not necessary to set a default message for bounces anymore.
Member

tpetr commented Jan 15, 2016

Thanks for the PR, Calvin!

A few notes:

  • It looks like some old expiring_actions commits bled into this branch. Now that the branch has been merged to master, could you rebase?
  • I think the original issue was unclear, sorry. When the new message field was added, the Task is cleaning banner was updated to only display the message value (which wasn't ideal, and is causing the blank "Task is cleaning:" banner you saw). What we want to do instead is for the banner to display cleanup.cleanupType, and the message in ( )'s only if there's one set. (examples: Task is cleaining: BOUNCING and Task is cleaning: BOUNCING (my custom message here))
  • With the new display mentioned above, it's not necessary to set a default message for bounces anymore.
Fixed to state cleanup.cleanupType - if there is a message, it will b…
…e in parenthesis after cleanup.cleanupType

@tpetr tpetr added this to the 0.4.9 milestone Jan 21, 2016

tpetr added a commit that referenced this pull request Jan 21, 2016

@tpetr tpetr merged commit 9a3adb6 into master Jan 21, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@ssalinas ssalinas deleted the task-is-cleaning-default-msg-fix branch Feb 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment