Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Adam B]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122691
---


Fix it, then Ship it!




Looks great! One more little bit of cleanup, and I'll commit it.


src/tests/persistent_volume_tests.cpp (line 683)


`StartMaster()` without any parameters will call `CreateMasterFlags()` 
anyway, so just drop the `CreateMasterFlags()`.


- Adam B


On March 7, 2016, 12:55 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 7, 2016, 12:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122465
---



Patch looks great!

Reviews applied: [44408]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 7, 2016, 8:55 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 7, 2016, 8:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

> On March 7, 2016, 7:39 p.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, lines 295-299
> > 
> >
> > Nit: An arrangement like this is preferable:
> > ```
> >   FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
> >   frameworkInfo.set_role("role1");
> >   
> >   MockScheduler sched;
> >   MesosSchedulerDriver driver(
> >   , frameworkInfo, master.get(), DEFAULT_CREDENTIAL);
> > ```

Thanks for the review Joseph. Just updated the review request, and also add 
Adam to the reviewer list. Let me know if there are any other issues.


- Yong


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122346
---


On March 7, 2016, 8:55 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 7, 2016, 8:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/
---

(Updated March 7, 2016, 8:52 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Move the placement of FrameworkInfo as suggested by Joseph.


Bugs: MESOS-4868
https://issues.apache.org/jira/browse/MESOS-4868


Repository: mesos


Description
---

This fix removes the setting up of ACLs in PersistentVolumeTests
as it is no longer needed any more with implicit roles (MESOS-4868).


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 

Diff: https://reviews.apache.org/r/44408/diff/


Testing
---

make check (in Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122346
---


Fix it, then Ship it!




LGTM.

You should probably ask Adam to shepherd since he committed the other implicit 
roles patches.


src/tests/persistent_volume_tests.cpp (lines 271 - 275)


Nit: An arrangement like this is preferable:
```
  FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
  frameworkInfo.set_role("role1");
  
  MockScheduler sched;
  MesosSchedulerDriver driver(
  , frameworkInfo, master.get(), DEFAULT_CREDENTIAL);
```


- Joseph Wu


On March 4, 2016, 8:17 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 4, 2016, 8:17 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122192
---



Patch looks great!

Reviews applied: [44408]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 5, 2016, 4:17 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 5, 2016, 4:17 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

> On March 5, 2016, 2:31 a.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, lines 283-284
> > 
> >
> > One more small refinement:
> > 
> > Now that the master does not use this variable, you should move it 
> > above the `MesosSchedulerDriver driver(...);`.
> > 
> > Ditto for all modified tests.

Thanks Joseph. Just updated the Review Request. Let me know if there are any 
other issues.


- Yong


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122173
---


On March 5, 2016, 4:17 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 5, 2016, 4:17 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/
---

(Updated March 5, 2016, 4:17 a.m.)


Review request for mesos and Joseph Wu.


Changes
---

Move the initialization of FrameworkInfo above MesosSchedulerDriver as 
FrameworkInfo is not needed for CreateMasterFlag.


Bugs: MESOS-4868
https://issues.apache.org/jira/browse/MESOS-4868


Repository: mesos


Description
---

This fix removes the setting up of ACLs in PersistentVolumeTests
as it is no longer needed any more with implicit roles (MESOS-4868).


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 

Diff: https://reviews.apache.org/r/44408/diff/


Testing
---

make check (in Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Guangya Liu]

> On 三月 5, 2016, 1:25 a.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, line 89
> > 
> >
> > This entire helper is not needed anymore (due to implicit roles).
> 
> Guangya Liu wrote:
> @Jopseph, why also remove acls here? The implicit roles will only create 
> roles on demand but what about acl?
> 
> Joseph Wu wrote:
> By default, ACLs are "permissive".  So if a given permission does not 
> exist, Mesos will allow it.  
> 
> For the majority of the tests that use this helper, they are only using 
> it to set initialize the `role` so that a framework can register.  The 
> ACL-specific tests set up ACLs separately.

Good to know, thanks @Joesph.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122163
---


On 三月 5, 2016, 2:24 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated 三月 5, 2016, 2:24 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122173
---



BTW, thanks for doing this :)


src/tests/persistent_volume_tests.cpp (lines 262 - 263)


One more small refinement:

Now that the master does not use this variable, you should move it above 
the `MesosSchedulerDriver driver(...);`.

Ditto for all modified tests.


- Joseph Wu


On March 4, 2016, 6:24 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 4, 2016, 6:24 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Joseph Wu]

> On March 4, 2016, 5:25 p.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, line 89
> > 
> >
> > This entire helper is not needed anymore (due to implicit roles).
> 
> Guangya Liu wrote:
> @Jopseph, why also remove acls here? The implicit roles will only create 
> roles on demand but what about acl?

By default, ACLs are "permissive".  So if a given permission does not exist, 
Mesos will allow it.  

For the majority of the tests that use this helper, they are only using it to 
set initialize the `role` so that a framework can register.  The ACL-specific 
tests set up ACLs separately.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122163
---


On March 4, 2016, 6:24 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 4, 2016, 6:24 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/
---

(Updated March 5, 2016, 2:24 a.m.)


Review request for mesos and Joseph Wu.


Changes
---

Removed entired MasterFlags() helper as it is not needed.


Bugs: MESOS-4868
https://issues.apache.org/jira/browse/MESOS-4868


Repository: mesos


Description
---

This fix removes the setting up of ACLs in PersistentVolumeTests
as it is no longer needed any more with implicit roles (MESOS-4868).


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 

Diff: https://reviews.apache.org/r/44408/diff/


Testing
---

make check (in Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Guangya Liu]

> On 三月 5, 2016, 1:25 a.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, line 89
> > 
> >
> > This entire helper is not needed anymore (due to implicit roles).

@Jopseph, why also remove acls here? The implicit roles will only create roles 
on demand but what about acl?


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122163
---


On 三月 4, 2016, 10:49 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated 三月 4, 2016, 10:49 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44408/#review122163
---




src/tests/persistent_volume_tests.cpp (line 89)


This entire helper is not needed anymore (due to implicit roles).


- Joseph Wu


On March 4, 2016, 2:49 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 4, 2016, 2:49 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>