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

browser.Dockerfile: Fix rm command #311

Merged
merged 2 commits into from
Oct 30, 2023
Merged

browser.Dockerfile: Fix rm command #311

merged 2 commits into from
Oct 30, 2023

Conversation

goekce
Copy link
Contributor

@goekce goekce commented Oct 28, 2023

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

@goekce goekce changed the title Remove files browser.Dockerfile: Fix rm command Oct 28, 2023
@marcdumais-work
Copy link
Contributor

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
Copy link
Contributor Author

goekce commented Oct 30, 2023

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
Copy link
Contributor

marcdumais-work commented Oct 30, 2023

@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 :)

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @goekce for this contribution!

@marcdumais-work marcdumais-work merged commit 764d9ba into eclipse-theia:master Oct 30, 2023
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.

Renamed directory paths in browser.Dockerfile
2 participants