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

nginx/setup-odk.sh: make file executable #840

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Dec 10, 2024

This change brings consistency: all other executables in files/** are marked executable in git rather than chmod'd in .dockerfile files.

The chmod call was originally introduced in #676 without discussion.

There is a wider debate whether git can be trusted to manage file permissions, touched on at #830 (comment)

This change brings consistency: all other executables in files/** are marked executable in git rather than chmod'd in dockerfiles.
@brontolosone
Copy link

I'm still of the opinion that it's better to define exactly the permissions you want and expect using mechanisms that are made for that exact purpose (eg chmod+chown, install, ...).

Git is unsuitable for that purpose; it only tracks executability, which is honored, or not, depending on the git client configuration and the OS, and the resulting user, group and others permissions are sensitive to the umask at clone time, which also makes for bugs such as this one.
Plus the recorded executability in git is not apparent if you don't happen to know where to look, contributing to a situation in which the resulting permissions become mystery meat sausage composed of interactions between several environmental factors.

I don't see any downsides to being explicit about intent and mechanism and desired outcome by putting a couple of install or chmod+chown calls in there. It closes the book on stated intent and on unexpected variance. And then if a file's permissions aren't exactly what you need or expect, then it's very easy to find the place where we define them.

@alxndrsn
Copy link
Contributor Author

I don't see any downsides to being explicit about intent and mechanism and desired outcome by putting a couple of install or chmod+chown calls in there.

I think the obvious downside is that it's more code to read, and could be considered noise if you weren't aware/concerned about environments with core.fileMode or less common umask values set. In addition to this, it's more code which is unlikely to be required most of the time, so unless there are explicit tests added for it, missing chmod calls are likely to be introduced in future, or existing ones likely to be removed when their intention can't be easily understood.

So I suggest merging this PR now to standardise executable flag handling across the repo. This could be followed up with another PR to add a test to CI which fails without chmod +x calls, and then add the required chmod calls.

@brontolosone brontolosone removed their request for review December 10, 2024 16:29
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.

2 participants