Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 16, 2017, 5:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 16, 2017, 5:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.hpp 7946f06cf17c05893ff80179c9d14b5f1f4023ef 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 
>   src/tests/master_validation_tests.cpp 
> fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

> On Feb. 16, 2017, 12:10 a.m., Vinod Kone wrote:
> > src/tests/slave_validation_tests.cpp, line 145
> > 
> >
> > Why is this test here instead of master validationt ests? Doesn't this 
> > validation happen in master?
> 
> Vinod Kone wrote:
> didn't intend this to be an issue. just curious.

Both the master and agent use that validation code - I picked the agent 
validation tests as the location somewhat arbitrarily.


- Greg


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


On Feb. 16, 2017, 5:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 16, 2017, 5:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.hpp 7946f06cf17c05893ff80179c9d14b5f1f4023ef 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 
>   src/tests/master_validation_tests.cpp 
> fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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

(Updated Feb. 16, 2017, 5:57 p.m.)


Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
Kone.


Changes
---

Updated comments, and changed `Secret.value.value` to `Secret.value.data`.


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


Repository: mesos


Description
---

This patch adds a field of type `Secret` to the
`Environment` protobuf message, enabling the passing
of secrets into the environments of executors and
tasks. Additional validation and test code is added
as well.


Diffs (updated)
-

  include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
  include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
  src/common/validation.hpp 7946f06cf17c05893ff80179c9d14b5f1f4023ef 
  src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
  src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 
  src/tests/master_validation_tests.cpp 
fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
  src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Vinod Kone]

> On Feb. 16, 2017, 12:10 a.m., Vinod Kone wrote:
> > src/tests/slave_validation_tests.cpp, line 145
> > 
> >
> > Why is this test here instead of master validationt ests? Doesn't this 
> > validation happen in master?

didn't intend this to be an issue. just curious.


- Vinod


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


On Feb. 15, 2017, 11:20 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 15, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.hpp 7946f06cf17c05893ff80179c9d14b5f1f4023ef 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 
>   src/tests/master_validation_tests.cpp 
> fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Vinod Kone]

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




include/mesos/v1/mesos.proto (line 1911)


Can you add a TODO here to remove the default in Mesos 2.1? Ideally, you 
should file a ticket with target version set to 2.1.



src/common/validation.cpp (line 154)


Can you add a NOTE here that if an user sets a new enum value (FOO) that 
this code doesn't know about yet (e.g., during upgrade), then instead of 
considering at as UNKNOWN we consider it as VALUE? I guess it is safe because 
that will most likely result in #156; unless the user mistakenly set type to 
`FOO` and `value` to non-empty.



src/common/validation.cpp (lines 168 - 175)


This should only happen when an user explicitly sets type as UNKNOWN. So we 
shouldn't check `has_value` but always return Error.



src/tests/slave_validation_tests.cpp (line 145)


Why is this test here instead of master validationt ests? Doesn't this 
validation happen in master?



src/tests/slave_validation_tests.cpp (lines 218 - 235)


this needs to be updated per the above comment.


- Vinod Kone


On Feb. 15, 2017, 11:20 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 15, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.hpp 7946f06cf17c05893ff80179c9d14b5f1f4023ef 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 
>   src/tests/master_validation_tests.cpp 
> fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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

(Updated Feb. 15, 2017, 11:20 p.m.)


Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
Kone.


Changes
---

Refactored tests and added null-byte secret validation.


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


Repository: mesos


Description
---

This patch adds a field of type `Secret` to the
`Environment` protobuf message, enabling the passing
of secrets into the environments of executors and
tasks. Additional validation and test code is added
as well.


Diffs (updated)
-

  include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
  include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
  src/common/validation.hpp 7946f06cf17c05893ff80179c9d14b5f1f4023ef 
  src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
  src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 
  src/tests/master_validation_tests.cpp 
fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
  src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

> On Feb. 9, 2017, 9:50 a.m., Adam B wrote:
> > LGTM, but you're mostly just updating the tests to uses VALUE, and there's 
> > only a single test for the REFERENCE type. Is there nothing else we should 
> > test there?
> 
> Greg Mann wrote:
> Since we haven't actually implemented the passing of secrets into 
> environments yet, validation tests are sufficient at the moment. My rationale 
> for including just one REFERENCE-type test was that since all of these 
> validation routines call into a common validation helper for `CommandInfo`:
> * validating a single secret type in all relevant protos ensures that the 
> common validation helper is being called in each case
> * validating the REFERENCE type in just one proto is sufficient to ensure 
> that the common helper validates that secret type correctly
> 
> We could validate all types in all cases, but I erred on the side of less 
> test code which accomplishes sufficient coverage. What do you think?

This is a bit more brittle, since changes to the validation implementation 
could poke holes in the testing coverage later.


- Greg


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


On Feb. 9, 2017, 6:34 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 9, 2017, 6:34 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
>   src/tests/master_validation_tests.cpp 
> 0c2649089d7fd29eb021ac75c71e6a74368577dc 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

> On Feb. 9, 2017, 9:50 a.m., Adam B wrote:
> > LGTM, but you're mostly just updating the tests to uses VALUE, and there's 
> > only a single test for the REFERENCE type. Is there nothing else we should 
> > test there?

Since we haven't actually implemented the passing of secrets into environments 
yet, validation tests are sufficient at the moment. My rationale for including 
just one REFERENCE-type test was that since all of these validation routines 
call into a common validation helper for `CommandInfo`:
* validating a single secret type in all relevant protos ensures that the 
common validation helper is being called in each case
* validating the REFERENCE type in just one proto is sufficient to ensure that 
the common helper validates that secret type correctly

We could validate all types in all cases, but I erred on the side of less test 
code which accomplishes sufficient coverage. What do you think?


- Greg


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


On Feb. 9, 2017, 6:34 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 9, 2017, 6:34 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
>   src/tests/master_validation_tests.cpp 
> 0c2649089d7fd29eb021ac75c71e6a74368577dc 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Adam B]

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



LGTM, but you're mostly just updating the tests to uses VALUE, and there's only 
a single test for the REFERENCE type. Is there nothing else we should test 
there?

- Adam B


On Feb. 8, 2017, 10:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 8, 2017, 10:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
>   src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
>   src/tests/master_validation_tests.cpp 
> 0c2649089d7fd29eb021ac75c71e6a74368577dc 
>   src/tests/slave_validation_tests.cpp 
> 3d17799ed04951fb56524db0f5d89347192300b2 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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

(Updated Feb. 9, 2017, 6:34 a.m.)


Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This patch adds a field of type `Secret` to the
`Environment` protobuf message, enabling the passing
of secrets into the environments of executors and
tasks. Additional validation and test code is added
as well.


Diffs (updated)
-

  include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
  include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
  src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
  src/tests/master_validation_tests.cpp 
0c2649089d7fd29eb021ac75c71e6a74368577dc 
  src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Vinod Kone]

> On Jan. 31, 2017, 8:31 p.m., Vinod Kone wrote:
> > include/mesos/v1/mesos.proto, line 1892
> > 
> >
> > ditto.
> 
> Greg Mann wrote:
> Sorry I'm not sure precisely what this issue is referring to? I added 
> text to this comment regarding just one of `value` and `secret` being set.

I meant my same comment above re: "comment should be removed" in mesos.proto 
applies here, v1/mesos.proto, as well.


- Vinod


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


On Feb. 2, 2017, 7:39 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 2, 2017, 7:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4 
>   src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50 
>   src/tests/master_validation_tests.cpp 
> edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

> On Jan. 31, 2017, 8:31 p.m., Vinod Kone wrote:
> > include/mesos/v1/mesos.proto, line 1892
> > 
> >
> > ditto.

Sorry I'm not sure precisely what this issue is referring to? I added text to 
this comment regarding just one of `value` and `secret` being set.


- Greg


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


On Feb. 1, 2017, 10:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Feb. 1, 2017, 10:08 p.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4 
>   src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50 
>   src/tests/master_validation_tests.cpp 
> edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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

(Updated Feb. 1, 2017, 10:08 p.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This patch adds a field of type `Secret` to the
`Environment` protobuf message, enabling the passing
of secrets into the environments of executors and
tasks. Additional validation and test code is added
as well.


Diffs (updated)
-

  include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
  include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
  src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
  src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4 
  src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50 
  src/tests/master_validation_tests.cpp 
edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
  src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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

(Updated Jan. 31, 2017, 10:44 p.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds a field of type `Secret` to the
`Environment` protobuf message, enabling the passing
of secrets into the environments of executors and
tasks. Additional validation and test code is added
as well.


Diffs (updated)
-

  include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
  include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
  src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
  src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Vinod Kone]

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




include/mesos/mesos.proto (lines 1909 - 1911)


This comment should be removed in this patch. It should just say "only one 
of these must be set."



include/mesos/v1/mesos.proto (line 1892)


ditto.



src/common/validation.cpp (lines 92 - 105)


If type is VALUE, value must be set. For UNKNOWN it's debatable because 
when new enum values are added that are not yet known to the validation code 
(upgrade case), it's not clear if we should expect `value` to be set.

Maybe we can do so:

```
if (type == SECRET)
  `secret` must be set

if (type == VALUE)
  `value` must be set
  
if (type == UNKNOWN)
   // Add a comment saying this is for backwards compatibility reasons
   `value` must be set

```



src/tests/slave_validation_tests.cpp (line 140)


I don't think we want to remove this constraint. See above.


- Vinod Kone


On Jan. 29, 2017, 5:10 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Jan. 29, 2017, 5:10 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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




src/common/validation.cpp (line 92)


I'll change these conditionals to a `switch` statement.


- Greg Mann


On Jan. 29, 2017, 5:10 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Jan. 29, 2017, 5:10 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55954, 55955, 56055, 56052, 56053]

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

- Mesos Reviewbot


On Jan. 29, 2017, 5:10 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56053/
> ---
> 
> (Updated Jan. 29, 2017, 5:10 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-7009
> https://issues.apache.org/jira/browse/MESOS-7009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a field of type `Secret` to the
> `Environment` protobuf message, enabling the passing
> of secrets into the environments of executors and
> tasks. Additional validation and test code is added
> as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56053/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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

(Updated Jan. 29, 2017, 5:10 a.m.)


Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds a field of type `Secret` to the
`Environment` protobuf message, enabling the passing
of secrets into the environments of executors and
tasks. Additional validation and test code is added
as well.


Diffs
-

  include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
  include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
  src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
  src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb 

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


Testing
---

`make check`


Thanks,

Greg Mann