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

[gp] env: handle multi-word values without quotes #4816

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

geropl
Copy link
Member

@geropl geropl commented Jul 14, 2021

Fixes #4736

  • /werft with-clean-slate-deployment

@geropl geropl requested review from a team and akosyakov and removed request for a team July 14, 2021 13:29
@geropl geropl force-pushed the geropl/gp-env-fails-when-value-4736 branch 2 times, most recently from 1352bdf to 594341e Compare July 14, 2021 14:32
@geropl
Copy link
Member Author

geropl commented Jul 14, 2021

@shaal If you have time: Could you double check that every case you discovered is covered here?

@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #4816 (9cad3ee) into main (edd30c0) will increase coverage by 9.74%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           main   #4816      +/-   ##
=======================================
+ Coverage      0   9.74%   +9.74%     
=======================================
  Files         0      14      +14     
  Lines         0     954     +954     
=======================================
+ Hits          0      93      +93     
- Misses        0     860     +860     
- Partials      0       1       +1     
Flag Coverage Δ
components-gitpod-cli-app 9.74% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/gitpod-cli/cmd/env.go 14.41% <100.00%> (ø)
components/gitpod-cli/cmd/sync-await.go 10.52% <0.00%> (ø)
components/gitpod-cli/cmd/sync-done.go 12.50% <0.00%> (ø)
components/gitpod-cli/cmd/preview.go 32.14% <0.00%> (ø)
components/gitpod-cli/cmd/url.go 36.36% <0.00%> (ø)
components/gitpod-cli/cmd/open.go 4.54% <0.00%> (ø)
components/gitpod-cli/cmd/init.go 1.88% <0.00%> (ø)
components/gitpod-cli/cmd/root.go 0.00% <0.00%> (ø)
components/gitpod-cli/cmd/gitpodRun.go 4.34% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edd30c0...9cad3ee. Read the comment docs.

@shaal
Copy link
Contributor

shaal commented Jul 14, 2021

@geropl I'd love helping testing this. But I would need steps I can follow to run tests on this PR.

I found https://werft.gitpod-dev.com/job/gitpod-build-geropl-gp-env-fails-when-value-4736.2, and I can get the Gitpod interface open.
But when I try opening a repo using that interface, I am getting this error message -

@geropl geropl force-pushed the geropl/gp-env-fails-when-value-4736 branch from 594341e to 9cad3ee Compare July 15, 2021 07:25
@geropl
Copy link
Member Author

geropl commented Jul 15, 2021

@shaal You're absolutely right, forgot about that I hav to unblock.

It would be awesome if you could double check the list of test cases here and suggest additions if any of your use-cases it missing. Thx!

Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Pending @shaal's check if the test cases cover the actual issue, this LGTM

@shaal
Copy link
Contributor

shaal commented Jul 15, 2021

@geropl when I look for an available environment I can test with here - https://werft.gitpod-dev.com, the only one that works is
https://werft.gitpod-dev.com/job/gitpod-build-geropl-gp-env-fails-when-value-4736.3
When I try running my repo on it, it says I am blocked.

When I try more recent environments, like https://werft.gitpod-dev.com/job/gitpod-wipe-devstaging-geropl-gp-env-fails-when-value-4736.8 - I don't see a way to open it to test.

Can you please create an environment I can use to test?

@csweichel
Copy link
Contributor

You can extract the updated gp command and use it in any workspace (e.g. prod) using:

docker run --rm -it --entrypoint=sh -v $PWD:/tmp/out eu.gcr.io/gitpod-core-dev/build/image-builder:commit-9cad3ee1e27e5d25e560d01294c8236f839c5a26 -c "mkdir /tmp/out/gplayer && cd /tmp/out/gplayer && tar xf /app/workspace-image-layer.tar.gz"

The gplayer/gitpod-cli binary is the new gp command that includes the fixes.

@geropl
Copy link
Member Author

geropl commented Jul 15, 2021

@shaal
Copy link
Contributor

shaal commented Jul 15, 2021

@geropl @csweichel Thank you!
I tried running all the test, as well as my use case gp env myssh="$(cat ~/.ssh/id_rsa)"
Everything works as expected, thank you for fixing that!

I don't know if this is related or not, there were 2 issues I noticed when unsetting an environment variable:

  1. When I unset an environment variable using gp env -u wow, no output is displayed to confirm the action (but perhaps this is by design, similar to how unset command works?)
  2. When I try to unset an environment variable that no longer exist gp env -u wow, I am getting this message
cannot unset wow: jsonrpc2: code 404 message: cannot delete 'wow' in scope 'shaal/drupalpod'

I think the error message can be simplified, but this is a very minor issue.

@geropl
Copy link
Member Author

geropl commented Jul 15, 2021

Thx for confirming!

Regarding 1: Yes, that's indeed meant to be scriptable.
Regarding 2: IMO this is a bug: it should be "no output, exitcode 0" as well, but I see our code tries to be explicit there.. worth another issue if you ask me.

@geropl geropl merged commit 63108b7 into main Jul 15, 2021
@geropl geropl deleted the geropl/gp-env-fails-when-value-4736 branch July 15, 2021 13:35
@shaal
Copy link
Contributor

shaal commented Jul 15, 2021

Thank you for working on it and fixing it so fast!

@geropl Do you know when will this fix get deployed to gitpod.io?
It will make my repo usable again 😁

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.

gp env - fails when value contains a space or newline
3 participants