feat: CLI: provide a way to list agents from the command line with filtering#986
feat: CLI: provide a way to list agents from the command line with filtering#986
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #986 +/- ##
==========================================
+ Coverage 75.31% 76.00% +0.68%
==========================================
Files 110 111 +1
Lines 10808 11172 +364
Branches 899 929 +30
==========================================
+ Hits 8140 8491 +351
- Misses 2474 2483 +9
- Partials 194 198 +4
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
…with negated statuses, e.g. --filter-status online,^waiting
…uments, tests, and documentation
Hook25
left a comment
There was a problem hiding this comment.
got nerd-sniped into giving this a review. Good job here, I have a few inline suggestions here and there but overall lgtm. Only major thing I would change is that I wouldn't put the help output in the howto due to bitrot, it is not very howto-ish and it is not quite clear what purpose it serves.
cli/testflinger_cli/__init__.py
Outdated
| dest="filter_comment", | ||
| help="Filter agents by comment (regex)", | ||
| ) | ||
| parser.add_argument( |
There was a problem hiding this comment.
this is a nargs with choice and an array default, please. Else the errors are wacky and late + you have to do more inline validation in the logic below. Also, use the default dumper if possible to print defaults in help.
There was a problem hiding this comment.
Following the spec for CLI, multi-value args should either use commas or repeated args. For these which have specific sets of values the preference is for a comma separated list.
I have moved this to it's own type parser for argparse.
There was a problem hiding this comment.
I think that is not idiomatic in any language as far as I know, so I would rather not follow such a bad guidance to be honest. Your call. I would just use nargs and ignore that.
ajzobro
left a comment
There was a problem hiding this comment.
Thanks for the review!
cli/testflinger_cli/__init__.py
Outdated
| dest="filter_comment", | ||
| help="Filter agents by comment (regex)", | ||
| ) | ||
| parser.add_argument( |
There was a problem hiding this comment.
Following the spec for CLI, multi-value args should either use commas or repeated args. For these which have specific sets of values the preference is for a comma separated list.
I have moved this to it's own type parser for argparse.
…rgparsing, incl. mutually exclusivity and reworked logic for filtering
…e parser, ensured that actually are constant as tests are run
tang-mm
left a comment
There was a problem hiding this comment.
Thanks for the addition! The new command is very well explained and organized. I left a couple of comments on the structure, please adjust them as needed
| cancel Tell the server to cancel a specified JOB_ID | ||
| config Get or set configuration options | ||
| jobs List the previously started test jobs | ||
| list-agents List agents with optional filtering |
There was a problem hiding this comment.
In the section "Check Available Queues on the Server" below, the tutorial shows usage of list-queues, but it only returns a few advertised queues. Does it make sense to switch to showing the new list-agents?
There was a problem hiding this comment.
@rene-oromtz What are your thoughts on swapping these (list-queues) vs (list-agents)?
There was a problem hiding this comment.
Two comments about this:
-
Based on a previous comment from Max, should we keep also the
--helphere? Would it be best to generalize and say if no error returned you are good to go? Otherwise whenever adding new commands, we need to update this (which is easy to forgot, at least I did for theadmincommand. -
As for the
Check Available Queues on the Serversection I think its a good idea to modify this section to make it actually useful with this new addition! We now have all the tools to find an appropriate queue for testing. We can now:
- Get a list of available agents first
list-agents(with filtering per data center, provision type etc) - Once you identify your
agent, you can get the queues the agent is listening withagent-status <agent> --json | jq .queues - Use any of those queues for your job.
We should eventually make list-queues to actually work, but at least right now we can provide value to users.
There was a problem hiding this comment.
Aaand solve one of the pain points in #661 (No example queue)
There was a problem hiding this comment.
#1 can be fixed with scripting (actions to keep the --help updated) -- I think the --help is useful (if up-to-date). I removed it from other places because those places were a 'how-to' and not a 'reference' section.
#2, just update --fields to show queues
Can update in that future ticket to output --json can still be used.
There was a problem hiding this comment.
- In this case, the help is just to indicate which are the options from the cli and to know it was installed successfully. For this purpose, I think we can keep a similar approach as juju docs tutorial to just say something similar as:
...
sudo snap install testflinger-cli
testflinger-cli 20260205 from Canonical Certification Team (ce-certification-qa) installed <- we can include this to explicitly let the user know the snap was installed successfully
...
Our testflinger-cli is ready! Take a quick look at what it can do: testflinger-cli --help <- we can add this to let the users know how to run the help in case they want to do something else not covered in the tutorial
that way no need to add another workflow or manage it manually but probably want @tang-mm inputs on this to see what is more maintainable on the long run.
- Sounds good, at least the general users should be able to use the new command to effectively get the queues. Once
--jsonis included, CI execution can leverage as well of this command output
Hook25
left a comment
There was a problem hiding this comment.
remember to purge the commented out code, consider the regex thingy which is pretty minor imo but a nice to have
There was a problem hiding this comment.
I left just a few comments but overall I think the feedback from Max and Meng Meng has been great and will let them give the final approval.
LGTM but In general, I think this could also benefit a lot if we do the filtering server side at the database level instead of client side e.g. GET /v1/agents/data with query parameters instead of getting all agents data and letting the client process it (it might be even faster). Not a hard requirement now because it will involve redesigning this PR and I think eventually we would need to make the API more robust anyway. Just a comment to maybe keep it as a future enhancement.
…agement, etc. in light of new list-agents command and uses thereof
There was a problem hiding this comment.
Thanks for the new pages, this were really needed! I focused on reviewing the docs changes that came from my earlier comments. I added some comments but I think overall for some of them I would want to see @tang-mm opinion on what is best.
docs/explanation/agents.rst
Outdated
| Agents in general | ||
| ----------------- | ||
|
|
There was a problem hiding this comment.
Since this is already an explanation for agents, I think this is not entirely required imo, consider removing this subsection
There was a problem hiding this comment.
Just checked below that your intention is to clarify the difference between agent vs agent host. Maybe this could use an introduction right below the H1 heading (line 5). Something that indicates the relation between agents and agent host. Then maybe this can just become:
| Agents in general | |
| ----------------- | |
| Agents | |
| ------ | |
| Agent Host Charms vs Agents | ||
| --------------------------- | ||
|
|
||
| Administrators of a Testflinger setup must understand the difference between an | ||
| Agent and the Agent Host Charms. The Agent Host Charm is managed within a Juju | ||
| environment is responsible for running the Agent itself. |
There was a problem hiding this comment.
Based on my previous comment this can maybe be generalized to indicate the relation between agent and agent hosts and uses as introduction to this page
docs/explanation/agents.rst
Outdated
| What is an Agent Host? | ||
| ~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Based on previous comments, this can then become:
| What is an Agent Host? | |
| ~~~~~~~~~~~~~~~~~~~~~~ | |
| Agent Host | |
| ---------- |
| The host machine on which the Agent Host Charm and thus the Agent run on. This | ||
| is different than the machine which the Agent's device connector will manage. | ||
|
|
||
| Agent Host Administration |
There was a problem hiding this comment.
This administration sectio is already covered in the new manage-agents.rst isnt? Maybe something pending to be removed? Not sure if something is missing but consider to review and check which things are needed to be removed/move to the new page
There was a problem hiding this comment.
We are in the agents explanation file. I don't know if it should EXPLAIN everything or if it should just point to HOW-TO documents where these things are demonstrated and also EXPLAINED again.
I will wait for @tang-mm to provide feedback before I make any changes because there are a lot of different ways we could do this and I don't want to draw this out guessing.
Co-authored-by: rene-oromtz <157750458+rene-oromtz@users.noreply.github.com>
Description
Adds
list-agentssub-command to the testflinger CLI to allow for listing agents which match specified criteria, and presenting that information either as a single column of the agents' name, a table of agent names and attributes, or a summary of the agent statuses matching the specified filter.There are three main modes of operation:
Single-column list mode (
-1)In single-column list mode (
-1) output is suitable for further processing by shell scripts in very much the same way asls -1:Table mode (default)
If neither
-1nor--summaryare specified, the default is to output a table of agents matching the specified filters.Summary mode (
--summary)Resolved issues
Resolves https://warthogs.atlassian.net/browse/CERTTF-884 (#982)
Resolves https://warthogs.atlassian.net/browse/CERTTF-778
Documentation
Updated agents.rst to include the
list-agentssubcommand.Web service API changes
N/A
Tests
Unit tests were added. See above for example usage as well.