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

Fix user account switching issue #137

Merged
merged 2 commits into from
Nov 29, 2022
Merged

Fix user account switching issue #137

merged 2 commits into from
Nov 29, 2022

Conversation

jcbhmr
Copy link
Contributor

@jcbhmr jcbhmr commented Nov 29, 2022

Fixes #136

In the future it might also be good to add more tests?

Jacob Hummer added 2 commits November 29, 2022 12:18
@jcbhmr jcbhmr changed the title Run as sudo to write to /etc/bash_completion.d/pulumi Fix user context-switching Nov 29, 2022
@jcbhmr jcbhmr changed the title Fix user context-switching Fix user context-switching issue Nov 29, 2022
@jcbhmr
Copy link
Contributor Author

jcbhmr commented Nov 29, 2022

I added two sudo prefixes, with comments, and also made sure to escape the $() that was testing for the command -v pulumi.

@jcbhmr jcbhmr changed the title Fix user context-switching issue Fix user account switching issue Nov 29, 2022
@danielbraun89 danielbraun89 merged commit 908892e into devcontainers-contrib:main Nov 29, 2022
@jcbhmr jcbhmr deleted the patch-1 branch November 29, 2022 19:44
@jcbhmr
Copy link
Contributor Author

jcbhmr commented Nov 29, 2022

I think I forgot to bump the version 🤦. My bad. @danielbraun89

fi

# finally we are adding bash completion. zsh support will be added soon
if [[ "${BASH_COMPLETION}" = "true" ]] ; then
pulumi gen-completion bash > /etc/bash_completion.d/pulumi
# We need sudo here since we are running under \$_REMOTE_USER
sudo pulumi gen-completion bash > /etc/bash_completion.d/pulumi
Copy link

Choose a reason for hiding this comment

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

Unfortunately, this sudo isn't enough, because it only runs the pulumi command as root. The redirection is done in userland, so it still fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drat! I don't even know how to accurately test this on my own local machine! Do you have any tips/advice so I can actually ensure/test/verify that these fixes that I'm trying work without publishing them to the ghcr.io registry?

Copy link

Choose a reason for hiding this comment

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

Yeah, that's a tough one. :/ Maybe something that spins up a container and runs the feature installer inside of it, would be similar to the real world scenario. I can give it some thought. :)

fi

# finally we are adding bash completion. zsh support will be added soon
if [[ "${BASH_COMPLETION}" = "true" ]] ; then
pulumi gen-completion bash > /etc/bash_completion.d/pulumi
# We need sudo here since we are running under \$_REMOTE_USER
sudo pulumi gen-completion bash > /etc/bash_completion.d/pulumi
Copy link

Choose a reason for hiding this comment

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

Suggested change
sudo pulumi gen-completion bash > /etc/bash_completion.d/pulumi
pulumi gen-completion bash | sudo tee /etc/bash_completion.d/pulumi

@jcbhmr
Copy link
Contributor Author

jcbhmr commented Nov 29, 2022

So it looks like the issue is still open. #136

To fix the permission error, there is more needed, as @fred-asimov says (and he is correct from a cursory SO glance)

Someone else suggested using dd to ✨ magically make it work

image

Also it looks like @fred-asimov suggests:

-sudo pulumi gen-completion bash > /etc/bash_completion.d/pulumi
+pulumi gen-completion bash | sudo tee /etc/bash_completion.d/pulumi

@danielbraun89 We/I need some more testing things to somehow actually try to catch these things. I'm making lots of mistakes! And mistakes are good when you have someone/some-robot to catch them, but I personally have no wish to push out non-functional code to people like @fred-asimov and @DaneWeber like I have done so far 😢. It's not very good.

Any suggestions anyone to help catch these earlier? Something in the test/ folder maybe? More robust shell testing?

@ghost
Copy link

ghost commented Nov 29, 2022

Any suggestions anyone to help catch these earlier? Something in the test/ folder maybe? More robust shell testing?

First off, don't sweat it, it's a hard-to-predict bug that's really difficult to test!

I think at a higher level, in terms of devcontainer fixtures, it would make sense to have an integration/smoke test (that runs on everything, not something that every feature has to maintain separately), which actually creates a devcontainer with the feature. I'm not entirely sure how to do that programmatically.

It might be sufficient to have a test that pulls a basic docker image, or even the base devcontainer image, and then checks out the feature and runs install.sh inside of it. That probably would have been enough to catch this. I can give that a try soon, and see if it's as easy as I'm claiming. 😆

@DaneWeber
Copy link
Contributor

Any suggestions anyone to help catch these earlier? Something in the test/ folder maybe? More robust shell testing?

I haven't looked into what's required, but I'd think supporting the following would Pretty Darn Good:

  • Run the tests as a non-root user (the REMOTE_USER such as vscode)
  • Provide a .devcontainer.json for each test suite. In particular, I'd like to run tests with defaults and another with specified values.
  • Besides looking for the check commands to run without error, it would be cool to run regex patterns against the output for higher-fidelity but resilient tests.

Some of these installs download a ton of data, so it might be important to cache the downloads somehow.

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.

Pulumi feature fails to install with bash completion
3 participants