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 an option to configure permission on dump #138

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

l00ptr
Copy link

@l00ptr l00ptr commented Jan 24, 2025

We add an option / configuration item to define the permissions for the dumps created with pg_back. We reuse / configure the 0600 permissions by default to ensure some compatibility with the old (non configurable) approach.

We probably need to discuss and improve what we should do for the parent directory when using 'd' for the dump format. Currently we still apply the 0700 mode in such situation, but maybe we should define a specific option or reuse the one we add right now.

@l00ptr l00ptr force-pushed the add-option-for-file-or-dump-permissions branch from 8c02a6a to fe5219b Compare January 24, 2025 17:06
@l00ptr l00ptr marked this pull request as draft January 24, 2025 17:07
@l00ptr
Copy link
Author

l00ptr commented Jan 24, 2025

Hi @orgrim,

This MR try to implement #132

I still want to improve it before some feedback about the code itself, but I think we should discuss about what we should do whith the parent directory when using the 'd' format and also we should also handle the situation where pg_back users simply want pg_back to use/respect the umask. What do you think we should do about those cases ?

Best regards,
Julian

@l00ptr l00ptr force-pushed the add-option-for-file-or-dump-permissions branch from fe5219b to d6e4304 Compare January 24, 2025 17:12
We add an option / configuration item to define the permissions for the
dumps created with pg_back. We reuse / configure the 0600 permissions by
default to ensure some compatibility with the old (non configurable)
approach.

We probably need to discuss and improve what we should do for the parent
directory when using 'd' for the dump format. Currently we still apply
the 0700 mode in such situation, but maybe we should define a specific
option or reuse the one we add right now.
@l00ptr l00ptr force-pushed the add-option-for-file-or-dump-permissions branch from d6e4304 to e774640 Compare January 24, 2025 17:31
@orgrim
Copy link
Owner

orgrim commented Jan 25, 2025

Hi @l00ptr, thanks a lot for taking care of this.

After a quick look: some lead on the default mode and how to "handle umask" would be to keep the current 0o600 mode for compatibility and use a negative value use to let the umask apply, since it ends up being store as a integer. What do you think?

On the directory format, I need to confirm if we are currently consistent on the permissions of all extra files, as this question applies to checksum files and others as well. I would expect the mode to be set once and applied everywhere.

While I need to test the patch, it looks promising.

Regards

@l00ptr
Copy link
Author

l00ptr commented Jan 27, 2025

After a quick look: some lead on the default mode and how to "handle umask" would be to keep the current 0o600 mode for compatibility and use a negative value use to let the umask apply, since it ends up being store as a integer. What do you think?

Hi @l00ptr, thanks a lot for taking care of this.

After a quick look: some lead on the default mode and how to "handle umask" would be to keep the current 0o600 mode for compatibility and use a negative value use to let the umask apply, since it ends up being store as a integer. What do you think?

good idea, done within the fixup.

On the directory format, I need to confirm if we are currently consistent on the permissions of all extra files, as this question applies to checksum files and others as well. I would expect the mode to be set once and applied everywhere.

I'll also look into how checksums (files) are generated and written (and how file permissions are set for them).

While I need to test the patch, it looks promising.

Regards

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