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

PLAT-1418 - Workaround fix for mount-data options showing as mandatry in the --help, while they are optional #484

Merged
merged 3 commits into from
Jan 31, 2025

Conversation

georgi-seqera
Copy link
Contributor

@georgi-seqera georgi-seqera commented Jan 31, 2025

Relates to : https://seqera.atlassian.net/browse/PLAT-1418

A known bug in the picocli framework causes even optional params that are grouped together to appear as mandatory in the --help section (as denoted by the *).

This causes the options to mount data to appear as mandatory, while they are optional.

Usage: tw studios start [OPTIONS]

Start a data studio.

Options:
  -w, --workspace=<workspace>       Workspace numeric identifier (TOWER_WORKSPACE_ID as default) or workspace reference as OrganizationName/WorkspaceName
* -i, --id=<sessionId>              DataStudio session ID.
* -n, --name=<dataStudioName>       DataStudio name.
*     --mount-data=<mountDataNames>[,<mountDataNames>...]
                                    Optional configuration override for 'mountData' setting (comma separate list of data-link names)
*     --mount-data-ids=<mountDataIds>[,<mountDataIds>...]
                                    Optional configuration override for 'mountData' setting (comma separate list of data-link Ids)
*     --mount-data-resource-refs=<mountDataResourceRefs>[,<mountDataResourceRefs>...]
                                    Optional configuration override for 'mountData' setting (comma separate list of data-link resource refs)
      --gpu=<gpu>                   Optional configuration override for 'gpu' setting (Integer representing number of cores)
      --cpu=<cpu>                   Optional configuration override for 'cpu' setting (Integer representing number of cores)
      --memory=<memory>             Optional configuration override for 'memory' setting (Integer representing memory in MBs)
      --wait=<wait>                 Wait until given status or fail. Valid options: starting, running, stopping, stopped, errored, building, buildFailed.
      --description=<description>   Optional configuration override for 'description'.
  -h, --help                        Show this help message and exit.
  -V, --version                     Print version information and exit.
$

A workaround (already used in the tower-cli for the PaginationOptions that experience the same issue ) is to add the (validation=false) option and implement our own validation.

After change:

Usage: tw studios start [OPTIONS]

Start a data studio.

Options:
  -w, --workspace=<workspace>       Workspace numeric identifier (TOWER_WORKSPACE_ID as default) or workspace reference as OrganizationName/WorkspaceName
* -i, --id=<sessionId>              DataStudio session ID.
* -n, --name=<dataStudioName>       DataStudio name.
      --gpu=<gpu>                   Optional configuration override for 'gpu' setting (Integer representing number of cores)
      --cpu=<cpu>                   Optional configuration override for 'cpu' setting (Integer representing number of cores)
      --memory=<memory>             Optional configuration override for 'memory' setting (Integer representing memory in MBs)
      --wait=<wait>                 Wait until given status or fail. Valid options: starting, running, stopping, stopped, errored, building, buildFailed.
      --description=<description>   Optional configuration override for 'description'.
  -h, --help                        Show this help message and exit.
  -V, --version                     Print version information and exit.
Option to mount data by passing in one of the below options:
      --mount-data=<mountDataNames>[,<mountDataNames>...]
                                    Optional configuration override for 'mountData' setting (comma separate list of data-link names)
      --mount-data-ids=<mountDataIds>[,<mountDataIds>...]
                                    Optional configuration override for 'mountData' setting (comma separate list of data-link Ids)
      --mount-data-resource-refs=<mountDataResourceRefs>[,<mountDataResourceRefs>...]
                                    Optional configuration override for 'mountData' setting (comma separate list of data-link resource refs)

Trying to supply more than one of the params still throws error:

$ ./tw studios start -w data-studios/data-studios -n studio-ef6b --mount-data adrian-navarro-test --mount-data-ids blabla

 ERROR: --mount-data=<mountDataNames>, --mount-data-ids=<mountDataIds>, --mount-data-resource-refs=<mountDataResourceRefs> are mutually exclusive (specify only one)

@georgi-seqera georgi-seqera marked this pull request as ready for review January 31, 2025 17:49
boolean idsProvided = mountDataIds != null;

// XOR function + check that not all 3 are provided covers that exactly 1 is provided
boolean valid = (namesProvided ^ resourceRefsProvided ^ idsProvided) && !(namesProvided && resourceRefsProvided && idsProvided);
Copy link
Member

Choose a reason for hiding this comment

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

minor: this is a bit too clever for my taste, but it checks out :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeaaah, I felt it as I was writing it too, but I just got excited at the opportunity to use the ^ operator :D

but I agree, it's not worth the weird complexity

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 revised this a bit to make it more readable

Copy link
Member

@endre-seqera endre-seqera left a comment

Choose a reason for hiding this comment

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

Cool solution!

@georgi-seqera georgi-seqera merged commit b28aa95 into master Jan 31, 2025
12 checks passed
@georgi-seqera georgi-seqera deleted the PLAT-1418-studio-checkpoints branch January 31, 2025 19:57
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.

2 participants