eclipse-ankaios / ankaios

Eclipse Ankaios provides workload and container orchestration for automotive High Performance Computing (HPC) software.
https://eclipse-ankaios.github.io/ankaios/
Apache License 2.0
60 stars 18 forks source link

Unify Ankaios CLI #173

Closed krucod3 closed 1 day ago

krucod3 commented 7 months ago

Description

Before the ank apply subcommand (#26) was implemented, all commands required a -f flag for reading files provided at the command line. The new apply subcommand takes a different approach there to improve usability.

To not make the usage of the ank CLI confusing, we should take care that the CLI behaves the same for all subcommands and change the old ones to use the new approach.

Goals

Unify the arguments taken by all ank subcommands to make the usage of the CLI easier and clearer.

Final result

Now, the ank CLI is unified and ank set state is being called in the same manner as ank apply.

Tasks

windsource commented 4 months ago

I think the only command that is affected by this would be ank set state, or?

HorjuRares commented 2 months ago

Currently working on this.

krucod3 commented 1 month ago

Shifted to 0.5 due to lack of time.

inf17101 commented 3 weeks ago

@krucod3, @windsource: I have noticed that we are now having two required positional arguments instead of one for set state after reviewing the PR. One or more object field masks and a new state file.

Previously, the new state file was an optional argument allowing a user to do the following: Executing ank set state desiredState without specifying a new state file and it deletes all workloads.

With the current changes in #340, the new state file is a required positional argument and is not optional anymore. Meaning the Option as internal type for the cli argument is useless (and the conditional checks on this type as well).

Secondly, with this change the usability has increased in the success case (no -f and similar to other ank commands), but it has decreased when the user does something wrong with the arguments.

This is because, before this change it was allowed to swap the arguments since the state file arg was a named argument.

Now, if a user accidentally swaps the arguments, then it tries to open the file on a field mask because the last argument is always treated as a file since it is now an positional argument: image

When we improve the error messages in the CLI this challenge comes back to us for the set state command.

Like discussed with @windsource, we bring the new changes to main.

HorjuRares commented 1 day ago

@krucod3

Final result

Now, the ank CLI is unified and ank set state is being called in the same manner as ank apply.

Tasks

krucod3 commented 1 day ago

All done here, thx @HorjuRares