Re: Review Request 55490: Used the `mesos/mesos-tidy` image from DockerHub.

[Benjamin Bannier]

> 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.

[Benjamin Bannier]

> On Jan. 18, 2017, 1:53 p.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 22
> > 
> >
> > 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.

[Michael Park]

> On Jan. 18, 2017, 4:53 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 22
> > 
> >
> > 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.

[Michael Park]

> On Jan. 18, 2017, 4:53 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 22
> > 
> >
> > 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.

[Benjamin Bannier]

---
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.

[Benjamin Bannier]

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




support/mesos-tidy.sh (line 22)


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.

[Michael Park]

---
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.

[Michael Park]

---
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.

[Michael Park]

---
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.

[Michael Park]

---
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.

[Michael Park]

> On Jan. 15, 2017, 1:55 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 43
> > 
> >
> > 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.

[Michael Park]

> On Jan. 15, 2017, 1:55 a.m., Benjamin Bannier wrote:
> > support/mesos-tidy.sh, line 43
> > 
> >
> > 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.

[Benjamin Bannier]

---
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)


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.

[Michael Park]

---
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.

[Michael Park]

---
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

Review Request 55490: Used the `mesos/mesos-tidy` image from DockerHub.

[Michael Park]

---
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