alces-software / adminware

A sandbox CLI for running commands remotely across nodes
1 stars 0 forks source link

Refactor `view *-status/*-history` into a unified status command #167

Closed WilliamMcCumstie closed 5 years ago

WilliamMcCumstie commented 5 years ago

Sorry for the largish PR, it couldn't be merged until the updates to pass_context where made. As a result a bunch of changes have been rolled together. I have made a few tweaks to #162 works and updated this PR to use command_creator.

The main purpose of the PR is to have a single status command that can intelligently determine if it is displaying a node or tool status. The change was requested in: https://alces.slack.com/archives/CCKGYQAL8/p1542020869217700

In practice this means it needs to be possible to filter by both node and tool (previously different commands had different filters). This required the use of subqueries that implement the different filters on the relevant Jobs. This in effect fixes #128 even if the query hasn't been abstracted.

The history commands have been replaced with a --history flag on status. This basically means run the SQL query without the group_by/count and then return all the jobs.

There has been a bit of internal refactoring concerning the sorting of Jobs. Previously they couldn't be sorted as the counts where stored in an independent array which relied on indexing the Jobs. Instead the count is stored on the corresponding Job which removes this issue.

The sort is now done by a custom __lt__ method on Job which follows the following rules:

  1. Sort by node name,
  2. Then sort by tool name,
  3. Then sort reverse chronologically

Finally the view result command has been absorbed into status --job JOB_ID. Otherwise this command is almost exactly the same.

The original view *-status/*-history result commands have been removed

WilliamMcCumstie commented 5 years ago

For your first two points, I have updated the status commands so they all take the --job input. They now have consistent behaviour between tool/node filtering.

For your third point I refer you to the sort order as described above. As we haven't got a requirement to sort chronologically, the standard sort order should be sufficient. Also typically reverse chronological order would be preferred.

status does break the "standard" verb-noun pattern of commands. However status was the requested command name in the requirement. There is a cmd_name variable that should(?) be able to change it with ease.

Finally I don't really care about deleted commands. A user deleting half of the commands would be equivalent to them dropping half of the db. We can expect this situation to lead to junk results.

DavidMarchant commented 5 years ago

Cheers!

For your third point I refer you to the sort order as described above. As we haven't got a requirement to sort chronologically, the standard sort order should be sufficient. Also typically reverse chronological order would be preferred.

Yep, absolutely, lets wait for this to be asked for

status does break the "standard" verb-noun pattern of commands. However status was the requested command name in the requirement. There is a cmd_name variable that should(?) be able to change it with ease.

As above! If someone wants to complain I'm sure that they will

Finally I don't really care about deleted commands. A user deleting half of the commands would be equivalent to them dropping half of the db. We can expect this situation to lead to junk results.

Yep, cool, this is definitely an unsupported eventuality

With that LGTM, merge through at your behest