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.

Sign up for GitHub

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

Jump to bottom

Change all file permissions to octal format #653

Merged
merged 2 commits into from
Apr 13, 2022
Merged

Change all file permissions to octal format #653

merged 2 commits into from
Apr 13, 2022

Conversation

peterfication
Copy link
Contributor

This better represent permissions as Linux handles such information in
octal format, meaning that the left-most 0 has an important meaning
and is not to be ignored as normally integers would.

See #603

@peterfication peterfication force-pushed the change-file-permissions-to-octal branch 2 times, most recently from 021e744 to e0f7d59 Compare April 10, 2022 09:51
@darkowlzz
Copy link
Contributor

Hi @peterfication, thanks for working on this. It looks good to me. Could you rebase it against the latest main branch?
If you don't need any more information regarding this, please mark it as ready for review.

@darkowlzz darkowlzz added the enhancement New feature or request label Apr 12, 2022
@hiddeco hiddeco requested a review from pjbgf April 12, 2022 12:03
@hiddeco
Copy link
Member

hiddeco commented Apr 12, 2022

@pjbgf can you go over this and see if there is room to lower some of the file and/or directory permissions?

pjbgf
pjbgf reviewed Apr 12, 2022
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

As a rule of thumb, the PermMode should be 0o660 (or less) for files and 0o770 (or less) for directories. Based on the common umask value of 0o022, files are created as 0o640 (or less) and directories 0o750 (or less).

Currently we rely on the machine set umask value, which may yield different results depending on OS/Distro. In the future we could pursue setting a controller level umask value to enforce 0o027 across test/runtime (i.e. syscall.Umask(0o027)).

main.go Outdated Show resolved Hide resolved
internal/helm/repository/chart_repository_test.go Outdated Show resolved Hide resolved
internal/helm/chart/metadata_test.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_local_test.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_local_test.go Outdated Show resolved Hide resolved
internal/fs/fs_test.go Outdated Show resolved Hide resolved
internal/fs/fs_test.go Outdated Show resolved Hide resolved
controllers/storage.go Outdated Show resolved Hide resolved
controllers/storage.go Outdated Show resolved Hide resolved
controllers/storage_test.go Outdated Show resolved Hide resolved
@peterfication peterfication force-pushed the change-file-permissions-to-octal branch from 1a29c07 to 858dd53 Compare April 12, 2022 19:18
@hiddeco
Copy link
Member

hiddeco commented Apr 12, 2022

@peterfication git fetch origin && git rebase origin/main && git push --force-with-lease should do the trick to get rid of the commits which are already in main. If origin is your fork, you need to replace it with the remote name for github.com/fluxcd/source-controller printed by git remote -v.

@peterfication
Change all file permissions to octal format
0949d0a 
This better represent permissions as Linux handles such information in
octal format, meaning that the left-most 0 has an important meaning
and is not to be ignored as normally integers would.

See fluxcd#603

Signed-off-by: Peter Gundel <mail@petergundel.de>
@peterfication peterfication force-pushed the change-file-permissions-to-octal branch 3 times, most recently from 1c5b435 to 625fb88 Compare April 12, 2022 19:28
@peterfication peterfication marked this pull request as ready for review April 12, 2022 19:29
pjbgf
pjbgf reviewed Apr 13, 2022
View reviewed changes
controllers/storage_test.go Outdated Show resolved Hide resolved
pjbgf
pjbgf reviewed Apr 13, 2022
View reviewed changes
controllers/storage_test.go Outdated Show resolved Hide resolved
pjbgf
pjbgf reviewed Apr 13, 2022
View reviewed changes
controllers/storage_test.go Outdated Show resolved Hide resolved
pjbgf
pjbgf requested changes Apr 13, 2022
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@peterfication I noticed that there are 5 occurrences in total in storage_test.go that could be changed from 0o755 to 0o750. Would you mind changing those too please?

I pointed out 3 on the review.

@peterfication peterfication force-pushed the change-file-permissions-to-octal branch from 625fb88 to 90d2291 Compare April 13, 2022 12:32
@peterfication
Copy link
Contributor Author

I just realized that there is one place left in gitrepository_fuzzer.go with a directory permission of 0o755. It was missed because it was already in octal format. I assume I should change this to 0o750 as well, right?

Also, there are a lot of places with permission 0o644. Should these all be changed to 0o640?

@pjbgf
Copy link
Member

pjbgf commented Apr 13, 2022

I just realized that there is one place left in gitrepository_fuzzer.go with a directory permission of 0o755. It was missed because it was already in octal format. I assume I should change this to 0o750 as well, right?

Also, there are a lot of places with permission 0o644. Should these all be changed to 0o640?

@peterfication yes, please, that would be awesome!

@peterfication @pjbgf
Reduce file permission
37551f1 
As suggested by @pjbgf

Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>

Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Signed-off-by: Peter Gundel <mail@petergundel.de>
@peterfication peterfication force-pushed the change-file-permissions-to-octal branch from 90d2291 to 37551f1 Compare April 13, 2022 15:15
@peterfication
Copy link
Contributor Author

Done

pjbgf
pjbgf approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

@peterfication great stuff, thank you for working on this. 🙇 🙇 🙇

LGTM

@pjbgf pjbgf merged commit a0070ce into fluxcd:main Apr 13, 2022
@peterfication peterfication deleted the change-file-permissions-to-octal branch April 13, 2022 16:50
@hiddeco hiddeco mentioned this pull request Apr 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjbgf pjbgf pjbgf approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@peterfication @darkowlzz @hiddeco @pjbgf