You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/01/29 05:10:20 UTC

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

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56053/#review163708
-----------------------------------------------------------




src/common/validation.cpp (line 92)
<https://reviews.apache.org/r/56053/#comment235187>

    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.

Posted by Vinod Kone <vi...@gmail.com>.

> On Jan. 31, 2017, 8:31 p.m., Vinod Kone wrote:
> > include/mesos/v1/mesos.proto, line 1892
> > <https://reviews.apache.org/r/56053/diff/1/?file=1618435#file1618435line1892>
> >
> >     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.

Posted by Greg Mann <gr...@mesosphere.io>.

> On Jan. 31, 2017, 8:31 p.m., Vinod Kone wrote:
> > include/mesos/v1/mesos.proto, line 1892
> > <https://reviews.apache.org/r/56053/diff/1/?file=1618435#file1618435line1892>
> >
> >     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.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56053/#review163712
-----------------------------------------------------------




include/mesos/mesos.proto (lines 1909 - 1911)
<https://reviews.apache.org/r/56053/#comment235193>

    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)
<https://reviews.apache.org/r/56053/#comment235194>

    ditto.



src/common/validation.cpp (lines 92 - 105)
<https://reviews.apache.org/r/56053/#comment235196>

    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)
<https://reviews.apache.org/r/56053/#comment235197>

    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.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
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.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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