Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Pass RUST_LOG to cli #2293

Merged
merged 3 commits into from
Oct 19, 2020
Merged

Pass RUST_LOG to cli #2293

merged 3 commits into from
Oct 19, 2020

Conversation

utopiabound
Copy link
Contributor

@utopiabound utopiabound commented Oct 1, 2020

iml-cli-proxy: calling direct prevents argument expansion
This also adds pathrough for RUST_LOG variable

Signed-off-by: Nathaniel Clark nclark@whamcloud.com


This change is Reviewable

Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
beevans
beevans previously approved these changes Oct 1, 2020
@jgrund
Copy link
Member

jgrund commented Oct 1, 2020

Let's use #2294 instead

@utopiabound utopiabound closed this Oct 1, 2020
@utopiabound utopiabound reopened this Oct 1, 2020
Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>

docker exec -i $TTYOPT iml_iml-manager-cli.1.$trailer iml "${@:1}"
docker exec -i $TTYOPT $ENVOPT iml_iml-manager-cli.1.$trailer /usr/bin/iml "${@:1}"
Copy link
Member

Choose a reason for hiding this comment

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

do we need to specify full path here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fixes the quote munging inside docker.

Copy link
Contributor

@beevans beevans Oct 1, 2020

Choose a reason for hiding this comment

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

Just trying this change out, I'm getting some odd behavior. iml to /usr/bin/iml gave me a "relative URL without a base" error.

Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but #2294 removes the other path entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

to do what you want here, you need docker exec -w /usr/bin -i ... iml ${@:1}"

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 guess the explicit full path for iml isn't required, but it doesn't hurt.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I run it by hand, it fails. So yes, it does.

@jgrund jgrund changed the title Fix docker cli quoting Pass RUST_LOG to cli Oct 1, 2020
@jgrund jgrund requested review from jgrund and beevans October 1, 2020 17:01
jgrund
jgrund previously approved these changes Oct 1, 2020
beevans
beevans previously approved these changes Oct 1, 2020
docker/iml-cli-proxy.sh Outdated Show resolved Hide resolved
Co-authored-by: Joe Grund <jgrund@whamcloud.io>
@utopiabound utopiabound dismissed stale reviews from beevans and jgrund via acd7336 October 16, 2020 13:41
Comment on lines +5 to +10
ENVOPT=""
if [ -n "$RUST_LOG" ]; then
ENVOPT="-e RUST_LOG=$RUST_LOG"
fi

docker exec -i $TTYOPT iml_iml-manager-cli.1.$trailer iml "${@:1}"
docker exec -i $TTYOPT $ENVOPT iml_iml-manager-cli.1.$trailer iml "${@:1}"
Copy link
Member

Choose a reason for hiding this comment

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

Nit.
Since we use bash anyway, to avoid any chance for problem with spaces:

ENVOPTS=()
if [ -n "$RUST_LOG" ]; then
	ENVOPTS+=( -e "RUST_LOG=$RUST_LOG" )
fi
docker exec -i $TTYOPT "${ENVOPTS[@]}" iml_iml-manager-cli.1.$trailer iml "${@:1}"

Could also construct the entire docker command this way.

@jgrund jgrund merged commit e24e054 into master Oct 19, 2020
@jgrund jgrund deleted the docker-iml-cli branch October 19, 2020 18:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants