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

Run now dialog improvements #977

Merged
merged 29 commits into from Apr 5, 2016
Merged

Run now dialog improvements #977

merged 29 commits into from Apr 5, 2016

Conversation

Calvinp
Copy link
Contributor

@Calvinp Calvinp commented Mar 29, 2016

This makes big changes to the run now dialog UX, and even bigger changes behind the scenes to enable this.

UX improvements

  • Fix a bug in which the run now button didn't work when clicked from the pending tasks page
  • The checkbox to automatically tail logs has been converted into radio buttons. The options are stay on the request's page (previous default behavior), tail logs, or browse to the task's page (new default option and what will be selected for you the first time you refresh after this is merged)
  • These radio buttons will remember your previous choice and default you to that choice
  • If you don't have automatically tail logs selected, the input box to specify a file to tail will be disabled
  • The input box to specify a file to tail will have 'service.log' filled in for you. If you write in something else, it will from then on fill that in for you instead. It uses local storage for this.
  • If you're automatically tailing logs, and the poller detects that the task you're polling is in a terminal state but the log you're looking for doesn't exist yet, it will give a more descriptive message in the popup.
  • Fix a bug in which the global constant defining the task poller timeout was completely ignored, and the timeout was instead set to one minute. But the user was told that the constant (5 minutes) was used, so it appeared to the user as if they had somehow lost 4 minutes of their life watching the poller.
  • Fix a bug in which the command line args weren't populated from localStorage
  • Change the command line args to populate from the last task run that request instead of localStorage
  • screenshot 2016-03-29 14 20 50

Behind the scenes UI changes

  • The code for the run now dialog was a huge mess riddled with bugs. This makes most of that better.
  • The promptRerun function in the request model has been vanquished. Instead, promptRun takes an optional task parameter, and if that parameter is specified it reruns the given task.
  • The logic to start the poller to automatically tail logs is now implemented by promptRun. It used to be that the caller would need to open the autotailer, and several callers didn't. So the autotailer wouldn't work if the task run button was pressed in certain places.
  • The poller to automatically detect when a task exists and its files exist totail logs has been enhanced. Now it takes a pollingType variable. If this type is 'autoTail', it does what it used to do. If it's 'browse-to-sandbox', it will instead direct you to a task's page when the task starts. It has been renamed from AutoTailer to TaskPoller, which is a much more accurate name for what it does. Several of its supporting templates have also been renamed. Many internal methods and variables have also been renamed. Unfortunately, with all the renamed variables, the diff is so different that Git wasn't able to detect that this is a rename - instead believing that these are all new files.
  • The poller also now uses runId to track a task rather than watching for a reset event, which sometimes missed certain tasks.

Backend change

  • @ssalinas added an API for retrieving running tasks by run id.

vex.dialog.prompt
message: "<h3>Run Task</h3>"
message: "<h3>#{if task then 'Rer' else 'R'}un Task</h3>"

Choose a reason for hiding this comment

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

nothing wrong with this logically, but for the sake of clarity in the code, maybe:

#{if task then 'Rerun' else 'Run'} Task

$('#filename').val localStorage.getItem('taskRunRedirectFilename')
$('#autoTail').prop 'checked', (localStorage.getItem('taskRunAutoTail') is 'on')
taskRunAfterStart = localStorage.getItem('taskRunAfterStart')
$('#filename').val localStorage.getItem('taskRunRedirectFilename') or 'service.log'
Copy link
Member

Choose a reason for hiding this comment

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

service.log is a very internal thing for us, if not using our executor the log file will be stdout or some other file. I believe we have this already somewhere, but there should be a configurable default log file we can put here instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It remembers your previous choice and puts it in there, so it might be ok to not actually have any default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I got that from the placeholder text, which is currently 'e.g. service.log'. But that's a constant, not configurable atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the configuration exists and is now being used.

<p><label class='label-light'><input id="autoTail" name="autoTail" type="checkbox"> Wait for task to start, then start tailing:</label></p>
<strong>After triggering the run:</strong>
<p>
<label class='label-light'><input id="stay-on-request-page" name="afterStart" type="radio" value="stay-on-request-page"> Stay on this request's page</label>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to take out the word "request's" here because the dialog can be accessed from pages other than just a request page.

message: "<h3>#{if task then 'Rerun' else 'Run'} Task</h3>"
input: runTemplate
id: @get "id"
prefix: @localStorageCommandLineInputKeyPrefix
Copy link
Member

Choose a reason for hiding this comment

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

do we still need this prefix if we aren't using localStorage for populating args anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! It's gone.

@trigger 'refreshrequest'
setTimeout ( => @trigger 'refreshrequest'), 2500
if taskId
task = new TaskHistory {taskId}
task.fetch()
Copy link
Member

Choose a reason for hiding this comment

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

we already have the task history fetched on this page, is there not a way we can use the data we already have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, that's not inherently true. While it is true if you're running from the individual request's page, you may also be running the request from the list of requests page, or from the pending tasks page.

Edited to clarify

"#{ config.apiRoot }/history/request/#{ @requestId }/run/#{ @runId }"
else
# Currently the above URL is the ONLY place to fetch this model.
# If you don't have access to request ID and run ID use the TaskHistory
Copy link
Member

Choose a reason for hiding this comment

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

once we have the taskId, we can also fetch this using that. I know the main use case with be by runId, but just wanted to clarify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you fetch by taskId, you get a different model as a response. That gives back a SingularityTaskHistory (which is the TaskHistory model in the UI), wheras searching by runId returns a SingularityTaskIdHistory (Which is this model in the UI). In a roundabout way, you can use Task Search to search out this model by taskId, but then you'd be fetching the collection rather than the model.

screenshot 2016-03-31 09 07 55
screenshot 2016-03-31 09 10 06

@Calvinp Calvinp added the hs_qa label Apr 1, 2016
@ssalinas ssalinas modified the milestone: 0.4.12 Apr 1, 2016
@ssalinas ssalinas merged commit 5ad8737 into master Apr 5, 2016
@ssalinas ssalinas deleted the run-now-dialog-improvements branch Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants