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

Add sudo to CBL-Mariner and AzureLinux distros #1179

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

liveans
Copy link
Member

@liveans liveans commented Aug 15, 2024

Adding sudo makes it easier to work on container.

@liveans liveans requested review from a team as code owners August 15, 2024 12:18
@liveans liveans enabled auto-merge August 15, 2024 13:49
@liveans liveans merged commit c9b1407 into dotnet:main Aug 15, 2024
38 checks passed
@richlander
Copy link
Member

Can you provide more detail as to why this is needed? We're trying to limit sudo if possible.

@sbomer

@liveans
Copy link
Member Author

liveans commented Aug 16, 2024

Can you provide more detail as to why this is needed? We're trying to limit sudo if possible.

In the container itself user is helixbot and without sudo, I couldn't easily install some stuff and do some system-wide changes. This is just needed to work on the container itself without needing to rebuild the image.
I'm totally fine to revert it, but it's intention is just make it easier to work with image.

@richlander
Copy link
Member

richlander commented Aug 16, 2024

I couldn't easily install some stuff and do some system-wide changes

Can you elaborate on why this is needed? If you need to install stuff, why not install those components in the actual image? Or if you are using the image locally, why not launch as root?

@liveans
Copy link
Member Author

liveans commented Aug 16, 2024

I couldn't easily install some stuff and do some system-wide changes

Can you elaborate on why this is needed? If you need to install stuff, why not install those components in the actual image? Or if you are using the image locally, why not launch as root?

microsoft/msquic#4466
We're basically adding our CI docker images into msquic pipeline to make sure that packaging works correctly and run simple .NET test on it, for that I'm not rebuilding image but using the publicly exposed one with script which does some changes on installed packages.
My first intention was to use images from this repo, but looks like it would make more sense to use https://hub.docker.com/r/microsoft/dotnet-runtime images.

So, I'm fine to revert this.

@richlander
Copy link
Member

If you can get away with using one of our product images, that is much better. They are a ton smaller.

Thanks. It would be best if we didn't include sudo in these images.

@MichaelSimons
Copy link
Member

It would be best if we didn't include sudo in these images.

I agree with this 100% - it would be good to have some checked in documentation with these types of guidelines.

@mthalman
Copy link
Member

Be aware that all of the Helix Dockerfiles install sudo. That's why I saw no issue with merging this, because these were the only two Helix Dockerfiles that were not doing that yet.

@liveans
Copy link
Member Author

liveans commented Aug 16, 2024

Be aware that all of the Helix Dockerfiles install sudo. That's why I saw no issue with merging this, because these were the only two Helix Dockerfiles that were not doing that yet.

I had the same impression as yours, and that's why I thought adding sudo into these images should be fine.

In that case, will there be work to remove sudo from other helix images?

@richlander
Copy link
Member

There is a slow path to tightening up our image create process (both content and the model for creating them). If sudo isn't needed, we shouldn't add it. Some of the more recent changes (driven by @jkoritzinsky) are really helping to limit the content in the images.

@MichaelSimons
Copy link
Member

The other thing to keep in mind are the changes @richlander is proposing in #997 where we separate out native build, managed build, and test images as they each have different requirements.

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.

4 participants