You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Zack Shoylev <no...@github.com> on 2015/04/23 06:34:29 UTC
[jclouds] Adds windows ACL code for file permissions, fixes build.
(#733)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/733
-- Commit Summary --
* Adds windows ACL code for file permissions, fixes build.
-- File Changes --
M apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java (220)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/733.patch
https://github.com/jclouds/jclouds/pull/733.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
> Path path = new File(buildPathStartingFromBaseDir(container)).toPath();
> - Set<PosixFilePermission> permissions;
> - try {
> - permissions = getPosixFilePermissions(path);
> - } catch (IOException ioe) {
> - throw Throwables.propagate(ioe);
> +
> + if ( System.getProperty("os.name", "").toLowerCase().contains("windows") ) {
Uops, didn't notice this was not a test. Forget about that comment.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733/files#r28964775
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
There is several repeated logic in this PR. Could you extract it to a WindowsUtils (or whatever) class, to encapsulate it there?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733#issuecomment-95598170
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Zack Shoylev <no...@github.com>.
About to rebase, merge, backport.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733#issuecomment-97171506
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Zack Shoylev <no...@github.com>.
> - try {
> - permissions = getPosixFilePermissions(path);
> - if (access == ContainerAccess.PRIVATE) {
> - permissions.remove(PosixFilePermission.OTHERS_READ);
> - } else if (access == ContainerAccess.PUBLIC_READ) {
> - permissions.add(PosixFilePermission.OTHERS_READ);
> +
> + if ( System.getProperty("os.name", "").toLowerCase().contains("windows") ) {
> + UserPrincipal everyone = null;
> + UserPrincipal me = null;
> + try {
> + everyone = getDefault().getUserPrincipalLookupService()
> + .lookupPrincipalByName("Everyone");
> + } catch (IOException e) {
> + e.printStackTrace();
> + return;
Yep, I messed up. I think we should propagate in both cases.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733/files#r28992122
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
Much cleaner. Thanks @zack-shoylev!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733#issuecomment-97331116
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Zack Shoylev <no...@github.com>.
merged and backported
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733#issuecomment-97181104
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
> Path path = new File(buildPathStartingFromBaseDir(container)).toPath();
> - Set<PosixFilePermission> permissions;
> - try {
> - permissions = getPosixFilePermissions(path);
> - } catch (IOException ioe) {
> - throw Throwables.propagate(ioe);
> +
> + if ( System.getProperty("os.name", "").toLowerCase().contains("windows") ) {
Extract this to a method in the [TestUtils](https://github.com/jclouds/jclouds/blob/master/core/src/test/java/org/jclouds/utils/TestUtils.java) class, similar to what we already do with OSX.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733/files#r28964697
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> + aclFileAttributes.setAcl(aclList);
> + } else {
> + List<AclEntry> list = aclFileAttributes.getAcl();
> + list.add(AclEntry.newBuilder().setPrincipal(everyone).setPermissions(
> + AclEntryPermission.READ_DATA,
> + AclEntryPermission.READ_ACL,
> + AclEntryPermission.READ_ATTRIBUTES,
> + AclEntryPermission.READ_NAMED_ATTRS)
> + .setType(AclEntryType.ALLOW)
> + .build());
> + aclFileAttributes.setAcl(list);
> + }
> + } catch (IOException e) {
> + e.printStackTrace();
> + return;
Same comments about exception handling.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733/files#r28965009
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
> - try {
> - permissions = getPosixFilePermissions(path);
> - if (access == ContainerAccess.PRIVATE) {
> - permissions.remove(PosixFilePermission.OTHERS_READ);
> - } else if (access == ContainerAccess.PUBLIC_READ) {
> - permissions.add(PosixFilePermission.OTHERS_READ);
> +
> + if ( System.getProperty("os.name", "").toLowerCase().contains("windows") ) {
> + UserPrincipal everyone = null;
> + UserPrincipal me = null;
> + try {
> + everyone = getDefault().getUserPrincipalLookupService()
> + .lookupPrincipalByName("Everyone");
> + } catch (IOException e) {
> + e.printStackTrace();
> + return;
When reading, if this exception happens, we propagate an exception, but when writing, we just silently return and ignore it? Can we change one or the other to have a consistent behavior?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733/files#r28964960
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Zack Shoylev <no...@github.com>.
It should be easy to refactor.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733#issuecomment-95685477
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
> - Set<PosixFilePermission> permissions;
> - try {
> - permissions = getPosixFilePermissions(path);
> - if (access == ContainerAccess.PRIVATE) {
> - permissions.remove(PosixFilePermission.OTHERS_READ);
> - } else if (access == ContainerAccess.PUBLIC_READ) {
> - permissions.add(PosixFilePermission.OTHERS_READ);
> +
> + if ( System.getProperty("os.name", "").toLowerCase().contains("windows") ) {
> + UserPrincipal everyone = null;
> + UserPrincipal me = null;
> + try {
> + everyone = getDefault().getUserPrincipalLookupService()
> + .lookupPrincipalByName("Everyone");
> + } catch (IOException e) {
> + e.printStackTrace();
Use a logger or remove this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733/files#r28964910
Re: [jclouds] Adds windows ACL code for file permissions, fixes
build. (#733)
Posted by Ignasi Barrera <no...@github.com>.
> Path path = new File(buildPathStartingFromBaseDir(container)).toPath();
> - Set<PosixFilePermission> permissions;
> - try {
> - permissions = getPosixFilePermissions(path);
> - } catch (IOException ioe) {
> - throw Throwables.propagate(ioe);
> +
> + if ( System.getProperty("os.name", "").toLowerCase().contains("windows") ) {
Anyway, extracting this to a method so it can be reused seems like a good idea.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/733/files#r28964847