You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/01/13 09:54:38 UTC
Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/
-----------------------------------------------------------
Review request for mesos and Benjamin Bannier.
Repository: mesos
Description
-------
See summary.
Diffs
-----
support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7
Diff: https://reviews.apache.org/r/55490/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review162095
-----------------------------------------------------------
Ship it!
Modulo my comments.
- Benjamin Bannier
On Jan. 18, 2017, 9:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 9:58 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 18, 2017, 1:53 p.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 22
> > <https://reviews.apache.org/r/55490/diff/6/?file=1607372#file1607372line22>
> >
> > Let's keep this further down so to not take away from `CHECKS` prominence. Any reason we'd want to deviate from `MESOS_DIRECTORY` used elsewhere? They seem to mean the same thing (extracted in slightly different ways).
>
> Michael Park wrote:
> I thought here was that most (if not all) of our scripts should start like this:
>
> ```bash
> #!/usr/bin/env bash
>
> // LICENSE ...
>
> set -e
> set -o pipefail
>
> MESOS_DIR=$(git rev-parse --show-toplevel)
> ```
>
> In terms of the use of `git`, I didn't like that I couldn't use
> `$(cd "$(dirname "$0")/.." && pwd)` in scripts within deeper directories
> (e.g., `support/jenkins`) because of the `..` portion of the pattern.
>
> It seems like the `git` command is more readable and also more portable.
>
> I did think about the "added" dependency on `git`, but `git` is an essential tool,
> and we would've needed `git` to end up with this repository somehow anyway.
> So I figured that shouldn't be an issue.
>
> Michael Park wrote:
> The `s/MESOS_DIRECTORY/MESOS_DIR/` is simply because we seem to use
> `_DIR` suffix for directories more widely than `DIRECTORY` in general.
Makes sense, dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review162068
-----------------------------------------------------------
On Jan. 18, 2017, 9:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 9:58 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 18, 2017, 4:53 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 22
> > <https://reviews.apache.org/r/55490/diff/6/?file=1607372#file1607372line22>
> >
> > Let's keep this further down so to not take away from `CHECKS` prominence. Any reason we'd want to deviate from `MESOS_DIRECTORY` used elsewhere? They seem to mean the same thing (extracted in slightly different ways).
>
> Michael Park wrote:
> I thought here was that most (if not all) of our scripts should start like this:
>
> ```bash
> #!/usr/bin/env bash
>
> // LICENSE ...
>
> set -e
> set -o pipefail
>
> MESOS_DIR=$(git rev-parse --show-toplevel)
> ```
>
> In terms of the use of `git`, I didn't like that I couldn't use
> `$(cd "$(dirname "$0")/.." && pwd)` in scripts within deeper directories
> (e.g., `support/jenkins`) because of the `..` portion of the pattern.
>
> It seems like the `git` command is more readable and also more portable.
>
> I did think about the "added" dependency on `git`, but `git` is an essential tool,
> and we would've needed `git` to end up with this repository somehow anyway.
> So I figured that shouldn't be an issue.
The `s/MESOS_DIRECTORY/MESOS_DIR/` is simply because we seem to use
`_DIR` suffix for directories more widely than `DIRECTORY` in general.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review162068
-----------------------------------------------------------
On Jan. 18, 2017, 12:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 12:58 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 18, 2017, 4:53 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 22
> > <https://reviews.apache.org/r/55490/diff/6/?file=1607372#file1607372line22>
> >
> > Let's keep this further down so to not take away from `CHECKS` prominence. Any reason we'd want to deviate from `MESOS_DIRECTORY` used elsewhere? They seem to mean the same thing (extracted in slightly different ways).
I thought here was that most (if not all) of our scripts should start like this:
```bash
#!/usr/bin/env bash
// LICENSE ...
set -e
set -o pipefail
MESOS_DIR=$(git rev-parse --show-toplevel)
```
In terms of the use of `git`, I didn't like that I couldn't use
`$(cd "$(dirname "$0")/.." && pwd)` in scripts within deeper directories
(e.g., `support/jenkins`) because of the `..` portion of the pattern.
It seems like the `git` command is more readable and also more portable.
I did think about the "added" dependency on `git`, but `git` is an essential tool,
and we would've needed `git` to end up with this repository somehow anyway.
So I figured that shouldn't be an issue.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review162068
-----------------------------------------------------------
On Jan. 18, 2017, 12:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 12:58 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review162068
-----------------------------------------------------------
support/mesos-tidy.sh (line 22)
<https://reviews.apache.org/r/55490/#comment233317>
Let's keep this further down so to not take away from `CHECKS` prominence. Any reason we'd want to deviate from `MESOS_DIRECTORY` used elsewhere? They seem to mean the same thing (extracted in slightly different ways).
- Benjamin Bannier
On Jan. 18, 2017, 9:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 9:58 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/
-----------------------------------------------------------
(Updated Jan. 18, 2017, 12:58 a.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Rebased.
Repository: mesos
Description (updated)
-------
Used the `mesos/mesos-tidy` image from DockerHub.
Diffs (updated)
-----
support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
Diff: https://reviews.apache.org/r/55490/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 6:34 p.m.)
Review request for mesos and Benjamin Bannier.
Repository: mesos
Description (updated)
-------
See summary.
Diffs
-----
support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
Diff: https://reviews.apache.org/r/55490/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 6:34 p.m.)
Review request for mesos and Benjamin Bannier.
Repository: mesos
Description (updated)
-------
Used the `mesos/mesos-tidy` image from DockerHub.
Diffs (updated)
-----
support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
Diff: https://reviews.apache.org/r/55490/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 3:56 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Removed `trap` for `docker rmi` as per Benjamin's comments.
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
Diff: https://reviews.apache.org/r/55490/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 15, 2017, 10:55 a.m., Benjamin Bannier wrote:
> > This is great. Could you make sure to follow up with setting `mesos/mesos-tidy` up for automated builds? Before building the image in the dockerhub time constraints was hard, but I am optimistic this would work now with the changes from https://reviews.apache.org/r/55488/.
FYI: I tried creating this docker image with an automated build, but also ran into the time limit.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review161660
-----------------------------------------------------------
On Jan. 18, 2017, 9:58 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2017, 9:58 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 96d3ecb91f5476ff499ca5f043c527681c30efe9
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 15, 2017, 1:55 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 43
> > <https://reviews.apache.org/r/55490/diff/3/?file=1605521#file1605521line43>
> >
> > Removing the image sure made sense for an always regenerated image, but does it also for a relatively static image? I as a developer would prefer to not always have to repull the image, but maybe e.g., a CI has different constraints.
> >
> > Let's keep the `docker pull` to ensure a recent enough image, but let's remove the `docker rmi`.
Yeah, I thought about this as well... I felt like leaving around `mesos/mesos-tidy` images in whatever jenkins CI machines we happen to land on would be bad practice and/or frowned upon.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review161660
-----------------------------------------------------------
On Jan. 14, 2017, 12:02 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2017, 12:02 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 15, 2017, 1:55 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 43
> > <https://reviews.apache.org/r/55490/diff/3/?file=1605521#file1605521line43>
> >
> > Removing the image sure made sense for an always regenerated image, but does it also for a relatively static image? I as a developer would prefer to not always have to repull the image, but maybe e.g., a CI has different constraints.
> >
> > Let's keep the `docker pull` to ensure a recent enough image, but let's remove the `docker rmi`.
>
> Michael Park wrote:
> Yeah, I thought about this as well... I felt like leaving around `mesos/mesos-tidy` images in whatever jenkins CI machines we happen to land on would be bad practice and/or frowned upon.
I've removed this. The vision here is that the scripts in the `support` directory should be standalone command-line tools, and the specific bot-scripts in the `jenkins` directory will simply use these standalone tools as it sees fit.
In this case, my plan is to remove the `docker rmi` from `support/mesos-tidy.sh` (as a command-line tool, we don't want to re-pull everytime) and add the `docker rmi` to `jenkins/tidybot.sh` since we want to be cleanly for the CI machines. Does this sound good to you?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review161660
-----------------------------------------------------------
On Jan. 14, 2017, 12:02 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2017, 12:02 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/#review161660
-----------------------------------------------------------
Fix it, then Ship it!
This is great. Could you make sure to follow up with setting `mesos/mesos-tidy` up for automated builds? Before building the image in the dockerhub time constraints was hard, but I am optimistic this would work now with the changes from https://reviews.apache.org/r/55488/.
support/mesos-tidy.sh (line 39)
<https://reviews.apache.org/r/55490/#comment232930>
Removing the image sure made sense for an always regenerated image, but does it also for a relatively static image? I as a developer would prefer to not always have to repull the image, but maybe e.g., a CI has different constraints.
Let's keep the `docker pull` to ensure a recent enough image, but let's remove the `docker rmi`.
- Benjamin Bannier
On Jan. 14, 2017, 9:02 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55490/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2017, 9:02 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the `mesos/mesos-tidy` image from DockerHub.
>
>
> Diffs
> -----
>
> support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7
>
> Diff: https://reviews.apache.org/r/55490/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/
-----------------------------------------------------------
(Updated Jan. 14, 2017, 12:02 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Used the `mesos/mesos-tidy` image from DockerHub.
Diffs (updated)
-----
support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7
Diff: https://reviews.apache.org/r/55490/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 55490: Used the `mesos/mesos-tidy` image from
DockerHub.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55490/
-----------------------------------------------------------
(Updated Jan. 13, 2017, 2 a.m.)
Review request for mesos and Benjamin Bannier.
Repository: mesos
Description (updated)
-------
Used the `mesos/mesos-tidy` image from DockerHub.
Diffs (updated)
-----
support/mesos-tidy.sh 98ea3857572942536c4f37579ec3f1cae64aaaf7
Diff: https://reviews.apache.org/r/55490/diff/
Testing
-------
Thanks,
Michael Park