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

feat: improve shell cli for optional args [DET-3883] [DET-3886] #1098

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

rb-determined-ai
Copy link
Member

BREKING CHANGE: Change the format of the ssh options arguments to
det shell start and det shell open. Instead of calling:

det shell start --ssh-opt "-XY -o SomeOption='some string'"

which has multiple layers of quotes due to a two-step shell tokenizing,
now users should call:

det shell start -- -XY -o SomeOption="some string"

This change also allows us to clean up our internal implementation to
avoid relying on shell tokenization internally.

Copy link
Contributor

@dzhu dzhu left a comment

Choose a reason for hiding this comment

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

Nice! This also fixes an issue I just discovered, which is that it was difficult at best to specify additional options starting with -o.

cli/determined_cli/shell.py Outdated Show resolved Hide resolved
@dzhu dzhu assigned rb-determined-ai and unassigned dzhu Aug 17, 2020
@dzhu
Copy link
Contributor

dzhu commented Aug 17, 2020

Also, commit/PR message: "BREKING" → "BREAKING".

BREAKING CHANGE: Change the format of the ssh options arguments to
`det shell start` and `det shell open`.  Instead of calling:

    det shell start --ssh-opt "-XY -o SomeOption='some string'"

which has multiple layers of quotes due to a two-step shell tokenizing,
now users should call:

    det shell start -- -XY -o SomeOption="some string"

This change also allows us to clean up our internal implementation to
avoid relying on shell tokenization internally.
@rb-determined-ai rb-determined-ai changed the title feat: improve shell cli for optional args feat: improve shell cli for optional args [DET-3883] [DET-3886] Aug 17, 2020
@rb-determined-ai
Copy link
Member Author

rb-determined-ai commented Aug 17, 2020

Also changing this PR to inject additional arguments after the ssh host when invoking ssh to solve DET-3886.

As a result, det shell open SHELL_ID -- -X -Y echo -n asdf works as expected: -X and -Y are consumed by ssh and -n is passed on to the echo command.

@dzhu
Copy link
Contributor

dzhu commented Aug 17, 2020

Looks good!

@dzhu dzhu assigned rb-determined-ai and unassigned dzhu Aug 17, 2020
@rb-determined-ai rb-determined-ai merged commit bde1641 into determined-ai:master Aug 17, 2020
@rb-determined-ai rb-determined-ai deleted the shell-cli branch August 17, 2020 23:22
rb-determined-ai pushed a commit that referenced this pull request Oct 27, 2023
* fix: update for error message change in product
rb-determined-ai pushed a commit that referenced this pull request Oct 31, 2023
* fix: update for error message change in product
rb-determined-ai pushed a commit that referenced this pull request Nov 2, 2023
* fix: update for error message change in product
rb-determined-ai pushed a commit that referenced this pull request Nov 2, 2023
* fix: update for error message change in product
@dannysauer dannysauer added this to the 0.13.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants