alces-flight / flight-asset-cli Goto Github PK

The component ID should be prompted for as "Component identifier" not "Asset identifier" (bug):

Asset Identifier:

The token doesn't need to be masked on input (enhancement):

Flight Center API token: •••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••

This was mentioned in #20 but needs to be handled as a separate issue.

The two option on how this could work:

1. Set the types as a configuration parameter with sensible defaults. This will allow supporting additional types without a code base change.

2. Dynamically set and cache the current valid types via a network request.

Add a command line option to the list-assets command to filter for the shown assets based on whether they've been decommissioned.

Perhaps flight asset list-assets --decommissioned <false|true|both>. Defaulting to both.

In addition to the global configuration file for static configuration values (#12) support should be added for a per-user configuration file for the static configuration. This configuration file should be located at ~/.config/flight/asset/config.yml.

The values in the per-user configure file, override those in the global configuration file. If any values are missing from both, they should default to the values given in #14.

AFAICS their has been an API change which leverages the full suite of HTTP status codes. The following is a log output that looks a bit funky:

# bin/asset move node01
I, [2020-08-12T14:57:11.434205 #2034]  INFO -- : Loaded Config: /host/asset-cli/etc/config.yaml
I, [2020-08-12T14:57:11.904205 #2034]  INFO -- : Running: FlightAsset::Commands::MoveAsset
W, [2020-08-12T14:57:11.962437 #2034]  WARN -- : Deprecated: the Config#component_id method should not be used
I, [2020-08-12T14:57:11.963270 #2034]  INFO -- request: GET https://staging.center.alces-flight.com/api/v1/assets?filter%5Bcomponent_id%5D=REDACTED&filter%5Bname%5D=node01
I, [2020-08-12T14:57:12.427773 #2034]  INFO -- response: Status 200
I, [2020-08-12T14:57:12.429031 #2034]  INFO -- request: GET https://staging.center.alces-flight.com/api/v1/assets?filter%5Bcomponent_id%5D=REDACTED&filter%5Bname%5D=node01
I, [2020-08-12T14:57:12.848823 #2034]  INFO -- response: Status 200
I, [2020-08-12T14:57:12.850260 #2034]  INFO -- request: PATCH https://staging.center.alces-flight.com/api/v1/assets/REDACTED/relationships/asset_group
I, [2020-08-12T14:57:13.070407 #2034]  INFO -- response: Status 403
I, [2020-08-12T14:57:13.070764 #2034]  INFO -- request: GET https://staging.center.alces-flight.com/api/v1/assets/REDACTED
I, [2020-08-12T14:57:13.464272 #2034]  INFO -- response: Status 200
I, [2020-08-12T14:57:13.466288 #2034]  INFO -- request: GET https://staging.center.alces-flight.com/api/v1/components/REDACTED
I, [2020-08-12T14:57:13.911699 #2034]  INFO -- response: Status 200
I, [2020-08-12T14:57:13.912975 #2034]  INFO -- request: GET https://staging.center.alces-flight.com/api/v1/asset-groups/REDACTED
I, [2020-08-12T14:57:14.354680 #2034]  INFO -- response: Status 200
                  Name: node01
          Support Type: managed (inherited)
             Component: Demo Cluster 1
           Asset Group: Demo Nodes
Additional Information: (none)
I, [2020-08-12T14:57:14.355216 #2034]  INFO -- : Exited: 0

Notably their was a 403 that was ignored by the rest of the command. This particular error wont be fixed as it is within a v1 move-* command. The v1 move-* commands are at EOL and are soon to be deprecated and replaced TBA.

However in general 40x errors (with exception of 404/ 421) are fatal as it's typically a credentialing issue. The other commands will require auditing for any uncaught error conditions and fixed appropriately.

A bash auto-completion script would be a nice but low priority enhancement. Perhaps for other shells too. Perhaps for other parts of the flight user suite too.

Rename the command, but don't change its behaviour. We may wish to change its behaviour at some point in the future.

The initial version of this tool worked around a bug in the FC asset creation API by sometimes creating assets in a "dummy group" and then removing them.

This workaround is confusing for a couple of reasons.

1. It results in the dummy group being visible in the FC UI. Which is confusing to anyone using FC who doesn't also have knowledge of this tool.
2. The presence of the dummy group in the configuration is confusing to engineers without a knowledge of the bug and its workaround.

Of course, a prerequisite is that the FC API supports creating groups without assigning them to a group. @benarmston is to investigate and fix that as required.

Ronseal

Ronseal

Similar to #2 but for the list-groups command.

Rework --info and --info-path s.t. in a similar way to curl you can do:

--info "this is my info string and it's lovely"

Or:

--info @/path/to/a/file

The mechanism here is that if the value of the --info option starts with an @ character it is to be treated as a path to a file containing the info. Otherwise it is to be treated as the info string itself.

Update the help text appropriately.

This change will make it harder to use the --info flag to set information beginning with the @ character. If a user wants the asset information to start with an @ character, they will need to either create a file containing the desired info and pass the path to that file or use funky bash magic. E.g.,

echo '@sset info is awesome' > /tmp/foo
flight asset update-asset my-asset --info @/tmp/foo

or alternatively

flight asset update-asset my-asset --info @<( echo '@asset info is awesome' )

The default behaviour of the update command is to throw a "missing" error if the asset does not already exist. This behaves as expected and should continue to be the case.

However there are cases where a script either wants to update and existing asset, or otherwise create it. To facilitate this, a new --create flag should be added to update. This flag should cause the update to fallback onto the create command if required.

Ideally the upstream API would support this feature, so the update/create could be done with a single request. However two requests are fine for an initial implementation.

Add limit to quantity of data that can be pumped through info field by default. Let’s limit it to 32KiB by default.

Make this a config file value. It should not be managed by the configure command. It should be overridable in the global and per-user config files.

The defaults for the configuration values that are not managed by flight asset configure should be changed to better support the tool being ran by non-root users.

The defaults should be as follows:

:base_url: https://center.alces-flight.com/api
:api_prefix: v1
:log_path: ~/.cache/flight/asset/flight-asset.log
:log_level: error
:tmp_path: /tmp/flight-asset
:credentials_file: ~/.config/flight/asset/credentials.yml

Similar to #8 but for groups and categories.

Ronseal

The UX of the configuration command could do with some love. The goal of the changes here are to make the configuration command behave in a more similar manner to the aws configure command.

The configuration is to be split into two categories: those managed by flight asset configure and those not managed by flight asset configure. These two groups of values are to be stored in two separate configuration files. Obviously, one of these is tool-managed and the other is not.

The values managed by the new configuration command are the component id and the API token.

The existing configure command should be replaced with one that takes no options and no arguments. That is flight asset configure. This should prompt the user for the token and component ID.

If present, the existing value for the configuration should be shown and truncated to 8 characters. If the value has not yet been shown, the string None should be used.

If the user does not enter a new value, or enters a new value consisting of only whitespace, the existing values should continue to be used.

Examples:

Initial configuration:

$ flight asset configure
Flight Center API token [None]: my-long-token
Component identifier [None]: 3

Update token:

$ flight asset configure
Flight Center API token [my-long-t...]: my-new-long-token
Component identifier [4]: <enter -- i.e. accepts default, leaves unchanged>

Other configuration values can be managed by the packaging system and/or advanced users by editing the config file. This, more static, config file should be at: ${flight_asset_ROOT}/etc/config.yml.

The decommissioned column should only be displayed only if the --include-decommissioned flag is present. By default, we show only commissioned assets as such the "decommissioned" column is just noise.

When displaying the decommissioned column use "yes"/"no" instead of "true"/"false".

Make the following changes to the help output.

• Change summary for edit-asset-info command from flight-asset edit-asset-info Asset to flight-asset edit-asset-info ASSET
• Change SUPPORT_TYPE to TYPE
• Change INFO_PATH to FILE
• Change ASSET_GROUP to GROUP, e.g.: flight-asset recommission-group ASSET_GROUP becomes flight-asset recommission-group GROUP.
• Correct flight-asset update-asset help text (allows modification of info as well):

  SYNOPSIS:

    flight-asset update-asset ASSET

  DESCRIPTION:

    Modify the support type for an asset

Also show possible values of support type in the help output. (This may not be simple and is perhaps suitable to be pulled out into its own issue).

Looks like this might be something to do with having to load all assets each time.

On the component I was trying to manipulate assets for there are ~800 assets and this causes flight asset list to take ~63 seconds to complete.

Similar times are seen for flight asset move.

Needs improving, cos I want to move several hundred assets. :-)

We should support creating and updating asset groups similar to how we update and create assets.

Note: asset categories are shared across all components currently. It is best that we don't support creating or updating them via a tool designed to manage a single component.

An engineer may accidentally decommission the wrong asset, perhaps an attempt to decommission node01 resulted in node10 being wrongly decommissioned. We should provide a command to undecommission an asset. Perhaps, this should be called commission or perhaps undecommission?

The credentials.yml file is created in the wrong place, i.e. it ends up at ~/.local/share/flight/asset-cli/credentials.yaml ("XDG_DATA") rather than ~/.config/flight/asset/credentials.yml ("XDG_CONFIG").

The list-assets and list-groups commands should support sorting the output by something other than name. Ideally we would support sorting by any attribute shown in the output. For list-assets, these would be name, support-type, decommissioned. For list-groups, these would be name, category, decommissioned.

If other attributes are added to the output, we should probably support sorting on them too.

The API supports sorting by multiple attributes. Bonus points awarded for supporting that too.

Note: the API currently expects the attribute name to in under_score format.

Ronseal.

It is currently broken and isn't really useful at the moment. If we think it will become useful again in the future we will re-add the command at that point.

[root@localhost vagrant]# /opt/flight/bin/flight asset show-category compute
flight-asset: uninitialized constant FlightAsset::Command::CategoriesMissing
Did you mean? FlightAsset::CategoryMissing
Did you mean? FlightAsset::CategoryMissing
[root@localhost vagrant]# /opt/flight/bin/flight asset show-category Compute
┌──────┬─────────┐
│ Name │ Compute │
└──────┴─────────┘

We currently, support listing all assets and listing all assets with a given group. It is not currently possible to list all assets that do not have a group. We should support that.

Instead of providing the new parent as an option use an argument. That is:

flight-asset move-asset ASSET GROUP

To remove an asset from its group, the following should be used

flight-asset move-asset ASSET ""

That is, use the empty string "" to remove the asset. We need to consider what the behaviour should be for an empty group string, e.g., " ".

Make similar changes to the move-group command. I.e.,

flight-asset move-group GROUP CATEGORY

This may be a restriction of the API, but it would be useful to distinguish between the situation where an asset is inheriting its support type and where it is explicitly set.

Once the new configure command is completed (#12) the set-token command should be dropped, as the new command will suffice.

Currently, the output of list-assets looks like:

+------------+-------------+--------------+
|Name        |Support Type |Decommissioned|
+------------+-------------+--------------+
|ben-asset-01|collaborative|false         |
|ben-asset-02|advice       |false         |
|ben-asset-03|advice       |false         |
|ben-asset-04|advice       |false         |
|ben-asset-05|managed      |false         |
+------------+-------------+--------------+

We should extend this to include the group name. E.g.,

+------------+-------------+-------+--------------+
|Name        |Support Type |Group  |Decommissioned|
+------------+-------------+-------+--------------+
|ben-asset-01|collaborative|Group 1|false         |
|ben-asset-02|advice       |Group 2|false         |
|ben-asset-03|advice       |Group 1|false         |
|ben-asset-04|advice       |Group 2|false         |
|ben-asset-05|managed      |Group 3|false         |
+------------+-------------+-------+--------------+

We should keep an eye on the performance cost of doing so. If costly, we might wish to make this opt in.

Flight Center does not (yet?) prevent multiple assets for a component from having the same name.

flight-asset does the right thing and refuses to create an asset with a duplicate name. However, there are ways of creating assets outside of flight-asset, so this is not sufficient to prevent duplicate assets.

If there are duplicate assets a request such as flight asset update-asset node01 --support-type managed is ill-defined. flight-asset should check for uniqueness and refuse to update the asset if it is not uniquely named.

There is a similar issue for groups, when listing the assets in a group. If there are multiple groups with the same name for the component, the results are ill-defined. Instead an error should be reported.

This is an unrelated issue that was mentioned in #23

When trying to display "excessively wide" info fields the tabling breaks. This is easiest to demonstrate with the update command:

# In an interactive terminal
> bin/asset update test2 --info @README.md
The table size exceeds the currently set width.To avoid error either. Defaulting to vertical orientation.
... hangs for awhile, then prints garbage ...
───────────────────────────────────────────────────────────────
───────────────────────────────────────────────────────────────────────────────────────────

This happens on any command that trys to print the info field (including show/create). It appears to do with TTY::Table not being able to pad/ format the large free form text. The non-interactive mode bypasses TTY::Table and therefore works fine:

> bin/asset update test2 --info @README.md | cat
test2                                                                                                   
advice                                                                                                  
Demo Cluster 1                                                                                          
                                                                                                        
no                                                  
# Flight Asset                                                                                          
                                                    
Manage Alces Flight Center Assets                                                                       
                                                                                                        
## Overview
... blah ...

The quick solution is skip rendering the info within TTY::Table and append it onto the end. This will be block text without any padding.

A a more longer term solution is not to display the info within a "show"/"update"/"create" at all (Instead the "info size" could be added?). Then a new view-asset would open the info according to $PAGER in a similar way that edit-asset opens the info in $VISUAL.

Ronseal

For consistency with other tools:

• Use unicode tables, cos they’re prettier.
• Use table padding -- o[:padding] = [0,1]

Currently "missing asset" and "missing group" errors both share the exit code 21. This causes problems because the update-asset command could error for either reason, making the code ambiguous.

The 21 exit code should be reserved for missing assets ONLY.

• flight-asset list-groups works fine.
• flight-asset list-groups --category '' works fine.
• flight-asset list-groups --category 'Compute' hangs. It gets stuck in an infinite loop requesting the same asset group category over and over.

The loop is caused when trying to print the asset group category name.