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

Preserving permissions prototype #195

Closed
wants to merge 3 commits into from
Closed

Preserving permissions prototype #195

wants to merge 3 commits into from

Conversation

ephess
Copy link

@ephess ephess commented Apr 2, 2012

Heya,

Sorry about the delay on this, been a busy few weeks at work.

I've got an initial prototype working which seems to avoid using a post install file to set permissions on the files after installation - what do you think of doing things this way?

I would be inclined to set the default owner/group to root and preserve the original file mode - do you agree with this? Currently the owner/group varies depending on whether fpm is run as a regular user or as root.

I then propose we add the 2 options you suggested --owner and --group with '-' for preserving current owner/group.

I do note that this change would only work with director as a source, are you happy with this?

Any feedback you have would be appreciated.

@ephess
Copy link
Author

ephess commented May 4, 2012

Wondering if you're going to get a chance to look at this soon? I'm going to have to continue work on it in this manner otherwise as I need to move ahead with the project I'll be using this functionality for.

@beddari
Copy link

beddari commented May 7, 2012

I'm also interested in this.

I think what you suggest ephess is OK other than I'd want preserving current owner/group to be the default, thus no special case for preserving with "-". I'd just use --owner root --group root as needed. In my mind that's simpler.

@ephess
Copy link
Author

ephess commented May 8, 2012

Although I agree with you that preserving the current permissions by default is more intuitive, it will be changing the way FPM behaves by default quite significantly which in my opinion is not in our best interest as it breaks backwards compatability. If jordansissel is happy to do this then I think it would be the best fix, however it is a rather intrusive change for anyone currently using fpm.

Jordan Hagan added 2 commits May 14, 2012 15:02
@ephess
Copy link
Author

ephess commented May 17, 2012

@jordansissel I've come up with a new method of preserving the file permissions using fakeroot, this makes fpm dependant on the fakeroot program however it is a much better way of achieving this in my opinion, it will also be a lot less work to pull this through to other package types (gem, rpm etc) than the previous method.

Looking forward to hearing your comments on this, I'll be rolling a patched version of fpm into production with this change in it so we'll see how we go in the coming weeks.

Once I've got your feedback on it and if you're happy to merge this / want more work done on it I'll tidy it up a bit, it's still very messy at the moment.

@llasram
Copy link
Contributor

llasram commented May 22, 2012

FYI, my pull request #217 to preserve file metadata has already been merged, along with a later fix in 823c510 by @jordansissel to handle over-zealous metadata-duplication on hardlinks.

@ephess
Copy link
Author

ephess commented May 22, 2012

Thanks for the heads up. Guess I'll close this one, I was under the impression that @jordansissel wanted fpm to be able to operate as a non root user otherwise that would have been a much easier fix :/

Cheers for letting me know.

@ephess ephess closed this May 22, 2012
@jordansissel
Copy link
Owner

I do want fpm to run as non-root, but #217 was a good step anyway since fpm wasn't properly copying things (it copied the files, not necessarily the metadata, but is now fixed).

Properly preserving permissions on the long term will require fpm to track file settings internally anyway, but I'm not sure how to best represent taht without making fpm too complex ;)

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.

None yet

4 participants