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/11/17 06:48:10 UTC
[jclouds] Fixes a windows locale bug with the "Everyone" principal
(#879)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/879
-- Commit Summary --
* Fixes a windows locale bug with the "Everyone" principal
-- File Changes --
M apis/filesystem/src/main/java/org/jclouds/filesystem/util/Utils.java (34)
M apis/filesystem/src/test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java (15)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/879.patch
https://github.com/jclouds/jclouds/pull/879.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Andrew Phillips <no...@github.com>.
> + Process pr = null;
> + try {
> + pr = rt.exec("whoami /groups | find \"S-1-1-0\"");
> + } catch (IOException e) {
> + return "Everyone";
> + }
> + try {
> + pr.waitFor();
> + } catch (InterruptedException e) {
> + e.printStackTrace();
> + }
> + try ( InputStreamReader reader = new InputStreamReader(pr.getInputStream(), Charsets.UTF_8)) {
> + return CharStreams.toString(reader).split(" ")[0];
> + } catch (IOException e) {
> + return "Everyone";
> + }
Could you explain this a bit? Why do we need the three blocks here? Could this be simplified into one block?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45024634
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Zack Shoylev <no...@github.com>.
> @@ -127,6 +128,20 @@ public void testCreateContainer() {
> TestUtils.directoryExists(TARGET_CONTAINER_NAME, true);
> }
>
> + public void testCreateContainerAccess() {
> + boolean result;
> +
> + TestUtils.directoryExists(TARGET_CONTAINER_NAME, false);
> + result = storageStrategy.createContainer(CONTAINER_NAME);
> + assertTrue(result, "Container not created");
> + TestUtils.directoryExists(TARGET_CONTAINER_NAME, true);
> +
> + storageStrategy.setContainerAccess(CONTAINER_NAME, ContainerAccess.PRIVATE);
> + assertEquals(storageStrategy.getContainerAccess(CONTAINER_NAME), ContainerAccess.PRIVATE);
> + storageStrategy.setContainerAccess(CONTAINER_NAME, ContainerAccess.PUBLIC_READ);
> + assertEquals(storageStrategy.getContainerAccess(CONTAINER_NAME), ContainerAccess.PUBLIC_READ);
> + }
Adds the missing test
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45198210
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Zack Shoylev <no...@github.com>.
merged
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879#issuecomment-158364902
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Andrew Gaul <no...@github.com>.
> + } catch (IOException e) {
> + return "Everyone";
> + }
> + try {
> + pr.waitFor();
> + } catch (InterruptedException e) {
> + e.printStackTrace();
> + }
> + try ( InputStreamReader reader = new InputStreamReader(pr.getInputStream(), Charsets.UTF_8)) {
> + return CharStreams.toString(reader).split(" ")[0];
> + } catch (IOException e) {
> + return "Everyone";
> + }
> + }
> +
> + public static String WINDOWS_EVERYONE = getWindowsEveryonePrincipalName();
Can we stub out this code on non-Windows?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45166208
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Andrew Gaul <no...@github.com>.
> @@ -100,12 +103,37 @@ public static void delete(File file) throws IOException {
> }
>
> /**
> + * @return Localized name for the "Everyone" Windows principal.
> + */
> + public static String getWindowsEveryonePrincipalName() {
Maybe just make this a `static` block?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45166276
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Zack Shoylev <no...@github.com>.
@andrewgaul @demobox I think I addressed everything but let me know if I missed something.
Also I should note this is related to
https://issues.apache.org/jira/browse/JCLOUDS-1015
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879#issuecomment-158034721
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Andrew Gaul <no...@github.com>.
> + */
> + public static String getWindowsEveryonePrincipalName() {
> + Runtime rt = Runtime.getRuntime();
> + Process pr = null;
> + try {
> + pr = rt.exec("whoami /groups | find \"S-1-1-0\"");
> + } catch (IOException e) {
> + return "Everyone";
> + }
> + try {
> + pr.waitFor();
> + } catch (InterruptedException e) {
> + e.printStackTrace();
> + }
> + try ( InputStreamReader reader = new InputStreamReader(pr.getInputStream(), Charsets.UTF_8)) {
> + return CharStreams.toString(reader).split(" ")[0];
Could you simplify this with:
```java
Process process = new ProcessBuilder("whoami /groups").start()
try {
String line;
while ((line = reader.readLine ()) != null) {
if (line.indexOf("\"S-1-1-0\"") != -1) {
return line.split(" ")[0];
}
}
} finally {
process.destroy();
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45166660
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Zack Shoylev <no...@github.com>.
Closed #879.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879#event-470270365
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Andrew Phillips <no...@github.com>.
> + } catch (IOException e) {
> + return "Everyone";
> + }
> + try {
> + pr.waitFor();
> + } catch (InterruptedException e) {
> + e.printStackTrace();
> + }
> + try ( InputStreamReader reader = new InputStreamReader(pr.getInputStream(), Charsets.UTF_8)) {
> + return CharStreams.toString(reader).split(" ")[0];
> + } catch (IOException e) {
> + return "Everyone";
> + }
> + }
> +
> + public static String WINDOWS_EVERYONE = getWindowsEveryonePrincipalName();
[minor] Can we make this `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45024589
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Zack Shoylev <no...@github.com>.
> @@ -100,12 +103,37 @@ public static void delete(File file) throws IOException {
> }
>
> /**
> + * @return Localized name for the "Everyone" Windows principal.
> + */
> + public static String getWindowsEveryonePrincipalName() {
I think it looks cleaner this way...?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45198573
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Andrew Phillips <no...@github.com>.
> + try {
> + String line;
> + try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
> + while ((line = reader.readLine()) != null) {
> + if (line.indexOf("S-1-1-0") != -1) {
> + return line.split(" ")[0];
> + }
> + }
> + }
> + } finally {
> + process.destroy();
> + }
> + } catch (IOException e) {
> + }
> + }
> + return "Everyone";
Fine to merge as-is - just a minor about perhaps commenting this as a fallback and/or default value?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45347072
Re: [jclouds] Fixes a windows locale bug with the "Everyone"
principal (#879)
Posted by Andrew Phillips <no...@github.com>.
> @@ -127,6 +128,20 @@ public void testCreateContainer() {
> TestUtils.directoryExists(TARGET_CONTAINER_NAME, true);
> }
>
> + public void testCreateContainerAccess() {
> + boolean result;
> +
> + TestUtils.directoryExists(TARGET_CONTAINER_NAME, false);
> + result = storageStrategy.createContainer(CONTAINER_NAME);
> + assertTrue(result, "Container not created");
> + TestUtils.directoryExists(TARGET_CONTAINER_NAME, true);
> +
> + storageStrategy.setContainerAccess(CONTAINER_NAME, ContainerAccess.PRIVATE);
> + assertEquals(storageStrategy.getContainerAccess(CONTAINER_NAME), ContainerAccess.PRIVATE);
> + storageStrategy.setContainerAccess(CONTAINER_NAME, ContainerAccess.PUBLIC_READ);
> + assertEquals(storageStrategy.getContainerAccess(CONTAINER_NAME), ContainerAccess.PUBLIC_READ);
> + }
Just curious - is this related to the change above, or just backfilling?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/879/files#r45024581