search

eclipse-theia / theia-ide

The Eclipse IDE is a modern and open IDE for cloud and desktop. The Theia IDE is based on the Theia platform. The Theia IDE is available as a downloadable desktop application. You can also try the latest version of the Theia IDE online. For more details, see the Readme below.
https://theia-ide.org/#theiaide
MIT License
343 stars 129 forks source link

`browser.Dockerfile`: Fix `rm` command #311

Closed goekce closed 12 months ago

goekce commented 12 months ago

What it does

Fixes wrong paths and removes an unnecessary option.

How to test

Rebuilding the Docker image

Review checklist

I don't know what thoroughly means however I could successfully build the Docker image.

closes #309

marcdumais-work commented 12 months ago

Hi @goekce,

Thanks for this PR.

remove -f because the listed directories will always exist \ This may help to catch future errors

I am not sure about this part. When I test locally on my laptop (not in Docker), running rm -f <non-existing directory>, a success status (0) is still returned. Also, I'm always a bit worries about permission issues with Docker, when there's a cross-over between host and container permissions, like in this case. Would you mind removing that commit?

goekce commented 12 months ago

rm -f <non-existing directory>, a success status (0) is still returned I believe there is a misunderstanding Marc. rm -f always succeeds and rm will output an error if a directory/file does not exist. This way you can catch future errors.

when there's a cross-over between host and container permissions

I don't understand this point, can you rephrase?

marcdumais-work commented 12 months ago

@goekce sorry - I made a mistake trying to test this locally during the weekend. You are right, rm -r will end with an error status if one or more of the folders do not exist, making it possible to catch a potential future name mismatch re-occurrence.

when there's a cross-over between host and container permissions

I don't understand this point, can you rephrase?

It was out of an abundance of caution, but it's easy to introduce file ownership permission issues when bringing host files into a container. e.g. if the uid of the user on the host is different than in the container. Then, depending on the umask the files had on the host, a different user/uid might be prompted before the files/folders can be removed, if rm is used without "f" (or they could be impossible to remove altogether without running chmod first. However in this case here, I believe the folders are removed by user root, and so it should not matter :)