Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
> On Nov. 12, 2015, 12:05 p.m., Zameer Manji wrote:
> > Although the end result LGTM, I'm not sure if having our own custom pants
> > plugin is the way to go here. Historically we have been very bad at
> > upgrading pants and maintaining it, I'm afraid that if we add a custom
> > plugin here we won't be able to upgrade pants in the future at all.
>
> John Sirois wrote:
> As you see fit. I will say that the APIs used here are minimal and
> historically stable. For example, Medium similarly enables checkstyle as
> well as another, non-default-installed plugin, `python-eval`, and they have
> not had to touch their plugin since initial install at 0.0.45 in August when
> it was released. The other bit to know is pants is locking down to 1.0.0 as
> the year closes to isolate more radical changes. There will be a long-term
> 1.0.0 branch the Square will be a major user of and a maintainer of with the
> primary focus being stability, secondary bugfixes, but ~no new public
> activity as the pants community charges on master towards 2.0.
>
> Zameer Manji wrote:
> Don't give up on this approach just yet, I'm curious on what the other
> reviewers have to say. I think putting the check in the pants layer is far
> better than what we have right now. I'm just not sure if we can maintain this
> going forward. If pants 1.0.0 will maintain this API that's pretty good to
> hear.
>
> John Sirois wrote:
> I did use more ceremony here than necessary. The plugin is just the
> single register.py file and the 2 entries in pants.ini. The BUILD files
> added and the maven-style directory tree in `build-support/plugins` are not
> needed. The BUILDs are only needed to support applying style checks, and
> adding tests to the custom plugin code - those could be dropped. The
> directory structure could be as simple as `build-support/pants_lugins` with
> an `__init__.py` and `register.py` in that dir and no more.
This review is being discussed on the pantsbuild slack and more discussion is
needed, but one other idea is for pants to ship the python checkstyle as a
plugin. This way folks who want to turn on checkstlye just add this to
pants.ini:
```ini
plugins: ['pantsbuild.pants.contrib.py_checkstyle==%(pants_version)s']
```
I'll provide an update when the discussion settles. Pants releases every
Friday, so if the community agrees on the plugin approach this could be ready
by tomorrow evening in `0.0.58`.
- John
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106301
---
On Nov. 12, 2015, 1:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 1:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/BUILD} | 18
> +++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/__init__.py} | 12
> +---
>
>
Re: Review Request 40249: [cli] Remove unused --local flag to job inspect command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40249/#review106317 --- Master (b4102de) is green with this patch. ./build-support/jenkins/build.sh However, it appears that it might lack test coverage. I will refresh this build result if you post a review containing "@ReviewBot retry" - Aurora ReviewBot On Nov. 12, 2015, 7:46 p.m., Paul Cavallaro wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40249/ > --- > > (Updated Nov. 12, 2015, 7:46 p.m.) > > > Review request for Aurora. > > > Repository: aurora > > > Description > --- > > [cli] Remove unused --local flag to job inspect command > > > Diffs > - > > src/main/python/apache/aurora/client/cli/jobs.py > 6dd9dec76a6cb2420469e4502ece60854b85155a > > Diff: https://reviews.apache.org/r/40249/diff/ > > > Testing > --- > > ./pants binary src/main/python/apache/aurora/client:aurora > ./dist/aurora.pex job inspect --help > ./dist/aurora.pex job inspect 'ed a config and looked correct > > > Didn't find any python_tests targets under the CLI [edit: incorrect] > > EDIT: > > Tested with: > ./pants test src/test/python/apache/aurora/client: > > Had failures in: > src/test/python/apache/aurora/client/cli/test_update.py > > Ran just those tests, and passed again, so perhaps flaky? > ./pants test src/test/python/apache/aurora/client/cli:update [passes] > > > Thanks, > > Paul Cavallaro > >
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
> On Nov. 12, 2015, 12:05 p.m., Zameer Manji wrote:
> > Although the end result LGTM, I'm not sure if having our own custom pants
> > plugin is the way to go here. Historically we have been very bad at
> > upgrading pants and maintaining it, I'm afraid that if we add a custom
> > plugin here we won't be able to upgrade pants in the future at all.
>
> John Sirois wrote:
> As you see fit. I will say that the APIs used here are minimal and
> historically stable. For example, Medium similarly enables checkstyle as
> well as another, non-default-installed plugin, `python-eval`, and they have
> not had to touch their plugin since initial install at 0.0.45 in August when
> it was released. The other bit to know is pants is locking down to 1.0.0 as
> the year closes to isolate more radical changes. There will be a long-term
> 1.0.0 branch the Square will be a major user of and a maintainer of with the
> primary focus being stability, secondary bugfixes, but ~no new public
> activity as the pants community charges on master towards 2.0.
>
> Zameer Manji wrote:
> Don't give up on this approach just yet, I'm curious on what the other
> reviewers have to say. I think putting the check in the pants layer is far
> better than what we have right now. I'm just not sure if we can maintain this
> going forward. If pants 1.0.0 will maintain this API that's pretty good to
> hear.
>
> John Sirois wrote:
> I did use more ceremony here than necessary. The plugin is just the
> single register.py file and the 2 entries in pants.ini. The BUILD files
> added and the maven-style directory tree in `build-support/plugins` are not
> needed. The BUILDs are only needed to support applying style checks, and
> adding tests to the custom plugin code - those could be dropped. The
> directory structure could be as simple as `build-support/pants_lugins` with
> an `__init__.py` and `register.py` in that dir and no more.
>
> John Sirois wrote:
> This review is being discussed on the pantsbuild slack and more
> discussion is needed, but one other idea is for pants to ship the python
> checkstyle as a plugin. This way folks who want to turn on checkstlye just
> add this to pants.ini:
> ```ini
> plugins: ['pantsbuild.pants.contrib.py_checkstyle==%(pants_version)s']
> ```
>
> I'll provide an update when the discussion settles. Pants releases every
> Friday, so if the community agrees on the plugin approach this could be ready
> by tomorrow evening in `0.0.58`.
>
> John Sirois wrote:
> Alrighty, the pantsbuild conclusion is pants should ship a plugin for
> py-checkstyle instead of embedding it in core pants, but turned off and
> needing a custom plugin to enable like I did here.
> Please hold off on review, I'll be updating the RB tomorrow to use the
> new plugin (I'm buildmeister this week, so I'm pretty confident the new
> plugin will ship tomorrow ;))
I'll actually discard this review and send up a new one when the py-checkstyle
plugin is released to avoid confusion.
- John
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106301
---
On Nov. 12, 2015, 1:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 1:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py}
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
> On Nov. 12, 2015, 12:05 p.m., Zameer Manji wrote:
> > Although the end result LGTM, I'm not sure if having our own custom pants
> > plugin is the way to go here. Historically we have been very bad at
> > upgrading pants and maintaining it, I'm afraid that if we add a custom
> > plugin here we won't be able to upgrade pants in the future at all.
>
> John Sirois wrote:
> As you see fit. I will say that the APIs used here are minimal and
> historically stable. For example, Medium similarly enables checkstyle as
> well as another, non-default-installed plugin, `python-eval`, and they have
> not had to touch their plugin since initial install at 0.0.45 in August when
> it was released. The other bit to know is pants is locking down to 1.0.0 as
> the year closes to isolate more radical changes. There will be a long-term
> 1.0.0 branch the Square will be a major user of and a maintainer of with the
> primary focus being stability, secondary bugfixes, but ~no new public
> activity as the pants community charges on master towards 2.0.
>
> Zameer Manji wrote:
> Don't give up on this approach just yet, I'm curious on what the other
> reviewers have to say. I think putting the check in the pants layer is far
> better than what we have right now. I'm just not sure if we can maintain this
> going forward. If pants 1.0.0 will maintain this API that's pretty good to
> hear.
>
> John Sirois wrote:
> I did use more ceremony here than necessary. The plugin is just the
> single register.py file and the 2 entries in pants.ini. The BUILD files
> added and the maven-style directory tree in `build-support/plugins` are not
> needed. The BUILDs are only needed to support applying style checks, and
> adding tests to the custom plugin code - those could be dropped. The
> directory structure could be as simple as `build-support/pants_lugins` with
> an `__init__.py` and `register.py` in that dir and no more.
>
> John Sirois wrote:
> This review is being discussed on the pantsbuild slack and more
> discussion is needed, but one other idea is for pants to ship the python
> checkstyle as a plugin. This way folks who want to turn on checkstlye just
> add this to pants.ini:
> ```ini
> plugins: ['pantsbuild.pants.contrib.py_checkstyle==%(pants_version)s']
> ```
>
> I'll provide an update when the discussion settles. Pants releases every
> Friday, so if the community agrees on the plugin approach this could be ready
> by tomorrow evening in `0.0.58`.
Alrighty, the pantsbuild conclusion is pants should ship a plugin for
py-checkstyle instead of embedding it in core pants, but turned off and needing
a custom plugin to enable like I did here.
Please hold off on review, I'll be updating the RB tomorrow to use the new
plugin (I'm buildmeister this week, so I'm pretty confident the new plugin will
ship tomorrow ;))
- John
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106301
---
On Nov. 12, 2015, 1:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 1:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
>
Re: Review Request 40220: Modernize the pex venv script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40220/ --- (Updated Nov. 12, 2015, 1:35 a.m.) Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman. Changes --- Modernize the pex venv script. This converts from grabbing the old `twitter.common.python` pex to grabbing modern pex to match the version specified in 3rdparty to help keep the pex venv up to date with the codebase dependencies. build-support/pex | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) Repository: aurora Description --- This converts from grabbing the old Twitter python pex to grabbing modern pex to match the version specified in 3rdparty to help keep the pex venv up to date with the codebase dependencies. build-support/pex | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) Diffs (updated) - build-support/pex 54e31f11f152cada3809ebd7b2cdec1d2ba12ac9 Diff: https://reviews.apache.org/r/40220/diff/ Testing --- Locally ran this 2x and observed both the proper version (1.1.0) and proper use of the cached venv in the second run: `git clean -fdx build-support && ./build-support/pex --version` Thanks, John Sirois
Re: Review Request 40220: Modernize the pex venv script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40220/#review106210 --- Sorry about that - diff 2 is what you want to look at: https://reviews.apache.org/r/40220/diff/2#index_header - John Sirois On Nov. 12, 2015, 1:35 a.m., John Sirois wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40220/ > --- > > (Updated Nov. 12, 2015, 1:35 a.m.) > > > Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman. > > > Repository: aurora > > > Description > --- > > This converts from grabbing the old Twitter python pex to grabbing > modern pex to match the version specified in 3rdparty to help keep the > pex venv up to date with the codebase dependencies. > > build-support/pex | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > > Diffs > - > > build-support/pex 54e31f11f152cada3809ebd7b2cdec1d2ba12ac9 > > Diff: https://reviews.apache.org/r/40220/diff/ > > > Testing > --- > > Locally ran this 2x and observed both the proper version (1.1.0) and > proper use of the cached venv in the second run: > `git clean -fdx build-support && ./build-support/pex --version` > > > Thanks, > > John Sirois > >
Re: Review Request 40220: Modernize the pex venv script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40220/#review106211 --- Ship it! Master (48aedae) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing "@ReviewBot retry" - Aurora ReviewBot On Nov. 12, 2015, 8:35 a.m., John Sirois wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40220/ > --- > > (Updated Nov. 12, 2015, 8:35 a.m.) > > > Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman. > > > Repository: aurora > > > Description > --- > > This converts from grabbing the old Twitter python pex to grabbing > modern pex to match the version specified in 3rdparty to help keep the > pex venv up to date with the codebase dependencies. > > build-support/pex | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > > Diffs > - > > build-support/pex 54e31f11f152cada3809ebd7b2cdec1d2ba12ac9 > > Diff: https://reviews.apache.org/r/40220/diff/ > > > Testing > --- > > Locally ran this 2x and observed both the proper version (1.1.0) and > proper use of the cached venv in the second run: > `git clean -fdx build-support && ./build-support/pex --version` > > > Thanks, > > John Sirois > >
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106207
---
Ship it!
Master (48aedae) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot
retry"
- Aurora ReviewBot
On Nov. 12, 2015, 8:01 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 8:01 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/BUILD} | 18
> +++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/__init__.py} | 12
> +---
>
> build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py
> | 36
> build-support/python/checkstyle
>| 34 --
> build-support/python/checkstyle-check
>| 6 +++---
> pants.ini
>| 43
> +++
> src/main/python/apache/aurora/admin/maintenance.py
>| 2 +-
> src/main/python/apache/aurora/client/api/__init__.py
>| 4 ++--
> src/main/python/apache/aurora/client/cli/client.py
>| 2 +-
> src/main/python/apache/aurora/client/cli/cron.py
>| 2 +-
> src/main/python/apache/thermos/core/process.py
>| 6 +++---
> src/main/python/apache/thermos/monitoring/process_collector_psutil.py
>| 1 -
> src/test/python/apache/aurora/admin/test_admin.py
>| 6 --
> src/test/python/apache/aurora/admin/util.py
>| 2 +-
> src/test/python/apache/aurora/client/cli/test_task.py
>| 3 +--
> 21 files changed, 111 insertions(+), 127 deletions(-)
>
>
> Diffs
> -
>
> build-support/hooks/pre-commit 619fa9e245be49e4e1f21781c0908cbf744b10ea
> build-support/jenkins/build.sh 5cd5242a2e4551e356e5ce5d15a0d27beb7e2d4e
> build-support/plugins/3rdparty/python/BUILD PRE-CREATION
> build-support/plugins/src/main/python/apache/__init__.py PRE-CREATION
> build-support/plugins/src/main/python/apache/aurora/__init__.py
> PRE-CREATION
> build-support/plugins/src/main/python/apache/aurora/pants/__init__.py
> PRE-CREATION
>
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106213
---
Ship it!
Master (48aedae) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot
retry"
- Aurora ReviewBot
On Nov. 12, 2015, 8:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 8:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/BUILD} | 18
> +++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/__init__.py} | 12
> +---
>
> build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py
> | 36
> build-support/python/checkstyle
>| 34 --
> build-support/python/checkstyle-check
>| 6 +++---
> pants.ini
>| 43
> +++
> src/main/python/apache/aurora/admin/maintenance.py
>| 2 +-
> src/main/python/apache/aurora/client/api/__init__.py
>| 4 ++--
> src/main/python/apache/aurora/client/cli/client.py
>| 2 +-
> src/main/python/apache/aurora/client/cli/cron.py
>| 2 +-
> src/main/python/apache/thermos/core/process.py
>| 6 +++---
> src/main/python/apache/thermos/monitoring/process_collector_psutil.py
>| 1 -
> src/test/python/apache/aurora/admin/test_admin.py
>| 6 --
> src/test/python/apache/aurora/admin/util.py
>| 2 +-
> src/test/python/apache/aurora/client/cli/test_task.py
>| 3 +--
> 21 files changed, 111 insertions(+), 127 deletions(-)
>
>
> Diffs
> -
>
> build-support/hooks/pre-commit 619fa9e245be49e4e1f21781c0908cbf744b10ea
> build-support/jenkins/build.sh 5cd5242a2e4551e356e5ce5d15a0d27beb7e2d4e
> build-support/plugins/3rdparty/python/BUILD PRE-CREATION
> build-support/plugins/src/main/python/apache/__init__.py PRE-CREATION
> build-support/plugins/src/main/python/apache/aurora/__init__.py
> PRE-CREATION
> build-support/plugins/src/main/python/apache/aurora/pants/__init__.py
> PRE-CREATION
>
Re: Review Request 40249: [cli] Remove unused --local flag to job inspect command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40249/ --- (Updated Nov. 12, 2015, 7:46 p.m.) Review request for Aurora. Changes --- Updated testing Repository: aurora Description --- [cli] Remove unused --local flag to job inspect command Diffs - src/main/python/apache/aurora/client/cli/jobs.py 6dd9dec76a6cb2420469e4502ece60854b85155a Diff: https://reviews.apache.org/r/40249/diff/ Testing (updated) --- ./pants binary src/main/python/apache/aurora/client:aurora ./dist/aurora.pex job inspect --help ./dist/aurora.pex job inspect 'ed a config and looked correct Didn't find any python_tests targets under the CLI [edit: incorrect] EDIT: Tested with: ./pants test src/test/python/apache/aurora/client: Had failures in: src/test/python/apache/aurora/client/cli/test_update.py Ran just those tests, and passed again, so perhaps flaky? ./pants test src/test/python/apache/aurora/client/cli:update [passes] Thanks, Paul Cavallaro
Re: Review Request 40249: [cli] Remove unused --local flag to job inspect command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40249/#review106359 --- Ship it! Ship It! - Bill Farner On Nov. 12, 2015, 11:46 a.m., Paul Cavallaro wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40249/ > --- > > (Updated Nov. 12, 2015, 11:46 a.m.) > > > Review request for Aurora. > > > Repository: aurora > > > Description > --- > > [cli] Remove unused --local flag to job inspect command > > > Diffs > - > > src/main/python/apache/aurora/client/cli/jobs.py > 6dd9dec76a6cb2420469e4502ece60854b85155a > > Diff: https://reviews.apache.org/r/40249/diff/ > > > Testing > --- > > ./pants binary src/main/python/apache/aurora/client:aurora > ./dist/aurora.pex job inspect --help > ./dist/aurora.pex job inspect 'ed a config and looked correct > > > Didn't find any python_tests targets under the CLI [edit: incorrect] > > EDIT: > > Tested with: > ./pants test src/test/python/apache/aurora/client: > > Had failures in: > src/test/python/apache/aurora/client/cli/test_update.py > > Ran just those tests, and passed again, so perhaps flaky? > ./pants test src/test/python/apache/aurora/client/cli:update [passes] > > > Thanks, > > Paul Cavallaro > >
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
> On Nov. 12, 2015, 11:05 a.m., Zameer Manji wrote:
> > Although the end result LGTM, I'm not sure if having our own custom pants
> > plugin is the way to go here. Historically we have been very bad at
> > upgrading pants and maintaining it, I'm afraid that if we add a custom
> > plugin here we won't be able to upgrade pants in the future at all.
>
> John Sirois wrote:
> As you see fit. I will say that the APIs used here are minimal and
> historically stable. For example, Medium similarly enables checkstyle as
> well as another, non-default-installed plugin, `python-eval`, and they have
> not had to touch their plugin since initial install at 0.0.45 in August when
> it was released. The other bit to know is pants is locking down to 1.0.0 as
> the year closes to isolate more radical changes. There will be a long-term
> 1.0.0 branch the Square will be a major user of and a maintainer of with the
> primary focus being stability, secondary bugfixes, but ~no new public
> activity as the pants community charges on master towards 2.0.
Don't give up on this approach just yet, I'm curious on what the other
reviewers have to say. I think putting the check in the pants layer is far
better than what we have right now. I'm just not sure if we can maintain this
going forward. If pants 1.0.0 will maintain this API that's pretty good to hear.
- Zameer
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106301
---
On Nov. 12, 2015, 12:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 12:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/BUILD} | 18
> +++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/__init__.py} | 12
> +---
>
> build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py
> | 36
> build-support/python/checkstyle
>| 34 --
> build-support/python/checkstyle-check
>| 6 +++---
> pants.ini
>| 43
> +++
> src/main/python/apache/aurora/admin/maintenance.py
>| 2 +-
> src/main/python/apache/aurora/client/api/__init__.py
>| 4 ++--
> src/main/python/apache/aurora/client/cli/client.py
>| 2 +-
> src/main/python/apache/aurora/client/cli/cron.py
>| 2 +-
>
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
> On Nov. 12, 2015, 12:05 p.m., Zameer Manji wrote:
> > Although the end result LGTM, I'm not sure if having our own custom pants
> > plugin is the way to go here. Historically we have been very bad at
> > upgrading pants and maintaining it, I'm afraid that if we add a custom
> > plugin here we won't be able to upgrade pants in the future at all.
>
> John Sirois wrote:
> As you see fit. I will say that the APIs used here are minimal and
> historically stable. For example, Medium similarly enables checkstyle as
> well as another, non-default-installed plugin, `python-eval`, and they have
> not had to touch their plugin since initial install at 0.0.45 in August when
> it was released. The other bit to know is pants is locking down to 1.0.0 as
> the year closes to isolate more radical changes. There will be a long-term
> 1.0.0 branch the Square will be a major user of and a maintainer of with the
> primary focus being stability, secondary bugfixes, but ~no new public
> activity as the pants community charges on master towards 2.0.
>
> Zameer Manji wrote:
> Don't give up on this approach just yet, I'm curious on what the other
> reviewers have to say. I think putting the check in the pants layer is far
> better than what we have right now. I'm just not sure if we can maintain this
> going forward. If pants 1.0.0 will maintain this API that's pretty good to
> hear.
I did use more ceremony here than necessary. The plugin is just the single
register.py file and the 2 entries in pants.ini. The BUILD files added and the
maven-style directory tree in `build-support/plugins` are not needed. The
BUILDs are only needed to support applying style checks, and adding tests to
the custom plugin code - those could be dropped. The directory structure could
be as simple as `build-support/pants_lugins` with an `__init__.py` and
`register.py` in that dir and no more.
- John
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106301
---
On Nov. 12, 2015, 1:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 1:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/BUILD} | 18
> +++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/__init__.py} | 12
> +---
>
> build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py
> | 36
> build-support/python/checkstyle
>| 34 --
> build-support/python/checkstyle-check
>| 6 +++---
> pants.ini
>| 43
>
Review Request 40249: [cli] Remove unused --local flag to job inspect command
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40249/ --- Review request for Aurora. Repository: aurora Description --- [cli] Remove unused --local flag to job inspect command Diffs - src/main/python/apache/aurora/client/cli/jobs.py 6dd9dec76a6cb2420469e4502ece60854b85155a Diff: https://reviews.apache.org/r/40249/diff/ Testing --- ./pants binary src/main/python/apache/aurora/client:aurora ./dist/aurora.pex job inspect --help ./dist/aurora.pex job inspect 'ed a config and looked correct Didn't find any python_tests targets under the CLI Thanks, Paul Cavallaro
Re: Review Request 40208: Eliminate OOB pip install of python deps in CI.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40208/#review106272 --- Ship it! Ship It! - Bill Farner On Nov. 11, 2015, 3:58 p.m., John Sirois wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40208/ > --- > > (Updated Nov. 11, 2015, 3:58 p.m.) > > > Review request for Aurora, Bill Farner and Zameer Manji. > > > Bugs: AURORA-954 > https://issues.apache.org/jira/browse/AURORA-954 > > > Repository: aurora > > > Description > --- > > Under older pants, and thus older pex, distributions were fetched using > urllib and were not robust to flaky connections. Now that Aurora is on > modern pants and pex, which uses the `requests` library for fetching and > includes 5 retries by default, this step should no longer be needed to > ensure stable CI runs. > > build-support/jenkins/build.sh | 7 --- > pants.ini | 5 - > 2 files changed, 12 deletions(-) > > > Diffs > - > > build-support/jenkins/build.sh 5cd5242a2e4551e356e5ce5d15a0d27beb7e2d4e > pants.ini 0bd8ec829ae65e2296a122166dea13f315323c9f > > Diff: https://reviews.apache.org/r/40208/diff/ > > > Testing > --- > > I still can't quite run `./build-support/jenkins/build.sh` straight up > due to https://issues.apache.org/jira/browse/AURORA-1083 but ran the > following successfully locally which forces re-download of requirements > by pants: > ``` > $ ./pants clean-all test.pytest --no-fast src/test/python:: > ``` > > > Thanks, > > John Sirois > >
Re: Review Request 40220: Modernize the pex venv script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40220/#review106267 --- Ship it! Ship It! - Bill Farner On Nov. 12, 2015, 12:35 a.m., John Sirois wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40220/ > --- > > (Updated Nov. 12, 2015, 12:35 a.m.) > > > Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman. > > > Repository: aurora > > > Description > --- > > This converts from grabbing the old Twitter python pex to grabbing > modern pex to match the version specified in 3rdparty to help keep the > pex venv up to date with the codebase dependencies. > > build-support/pex | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > > Diffs > - > > build-support/pex 54e31f11f152cada3809ebd7b2cdec1d2ba12ac9 > > Diff: https://reviews.apache.org/r/40220/diff/ > > > Testing > --- > > Locally ran this 2x and observed both the proper version (1.1.0) and > proper use of the cached venv in the second run: > `git clean -fdx build-support && ./build-support/pex --version` > > > Thanks, > > John Sirois > >
Re: Review Request 40208: Eliminate OOB pip install of python deps in CI.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40208/#review106297 --- Ship it! Ship It! - Zameer Manji On Nov. 11, 2015, 3:58 p.m., John Sirois wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40208/ > --- > > (Updated Nov. 11, 2015, 3:58 p.m.) > > > Review request for Aurora, Bill Farner and Zameer Manji. > > > Bugs: AURORA-954 > https://issues.apache.org/jira/browse/AURORA-954 > > > Repository: aurora > > > Description > --- > > Under older pants, and thus older pex, distributions were fetched using > urllib and were not robust to flaky connections. Now that Aurora is on > modern pants and pex, which uses the `requests` library for fetching and > includes 5 retries by default, this step should no longer be needed to > ensure stable CI runs. > > build-support/jenkins/build.sh | 7 --- > pants.ini | 5 - > 2 files changed, 12 deletions(-) > > > Diffs > - > > build-support/jenkins/build.sh 5cd5242a2e4551e356e5ce5d15a0d27beb7e2d4e > pants.ini 0bd8ec829ae65e2296a122166dea13f315323c9f > > Diff: https://reviews.apache.org/r/40208/diff/ > > > Testing > --- > > I still can't quite run `./build-support/jenkins/build.sh` straight up > due to https://issues.apache.org/jira/browse/AURORA-1083 but ran the > following successfully locally which forces re-download of requirements > by pants: > ``` > $ ./pants clean-all test.pytest --no-fast src/test/python:: > ``` > > > Thanks, > > John Sirois > >
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106301
---
Although the end result LGTM, I'm not sure if having our own custom pants
plugin is the way to go here. Historically we have been very bad at upgrading
pants and maintaining it, I'm afraid that if we add a custom plugin here we
won't be able to upgrade pants in the future at all.
- Zameer Manji
On Nov. 12, 2015, 12:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 12:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/BUILD} | 18
> +++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/__init__.py} | 12
> +---
>
> build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py
> | 36
> build-support/python/checkstyle
>| 34 --
> build-support/python/checkstyle-check
>| 6 +++---
> pants.ini
>| 43
> +++
> src/main/python/apache/aurora/admin/maintenance.py
>| 2 +-
> src/main/python/apache/aurora/client/api/__init__.py
>| 4 ++--
> src/main/python/apache/aurora/client/cli/client.py
>| 2 +-
> src/main/python/apache/aurora/client/cli/cron.py
>| 2 +-
> src/main/python/apache/thermos/core/process.py
>| 6 +++---
> src/main/python/apache/thermos/monitoring/process_collector_psutil.py
>| 1 -
> src/test/python/apache/aurora/admin/test_admin.py
>| 6 --
> src/test/python/apache/aurora/admin/util.py
>| 2 +-
> src/test/python/apache/aurora/client/cli/test_task.py
>| 3 +--
> 21 files changed, 111 insertions(+), 127 deletions(-)
>
>
> Diffs
> -
>
> build-support/hooks/pre-commit 619fa9e245be49e4e1f21781c0908cbf744b10ea
> build-support/jenkins/build.sh 5cd5242a2e4551e356e5ce5d15a0d27beb7e2d4e
> build-support/plugins/3rdparty/python/BUILD PRE-CREATION
> build-support/plugins/src/main/python/apache/__init__.py PRE-CREATION
> build-support/plugins/src/main/python/apache/aurora/__init__.py
> PRE-CREATION
>
Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
> On Nov. 12, 2015, 12:05 p.m., Zameer Manji wrote:
> > Although the end result LGTM, I'm not sure if having our own custom pants
> > plugin is the way to go here. Historically we have been very bad at
> > upgrading pants and maintaining it, I'm afraid that if we add a custom
> > plugin here we won't be able to upgrade pants in the future at all.
As you see fit. I will say that the APIs used here are minimal and
historically stable. For example, Medium similarly enables checkstyle as well
as another, non-default-installed plugin, `python-eval`, and they have not had
to touch their plugin since initial install at 0.0.45 in August when it was
released. The other bit to know is pants is locking down to 1.0.0 as the year
closes to isolate more radical changes. There will be a long-term 1.0.0 branch
the Square will be a major user of and a maintainer of with the primary focus
being stability, secondary bugfixes, but ~no new public activity as the pants
community charges on master towards 2.0.
- John
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40219/#review106301
---
On Nov. 12, 2015, 1:54 a.m., John Sirois wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> ---
>
> (Updated Nov. 12, 2015, 1:54 a.m.)
>
>
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and
> Zameer Manji.
>
>
> Bugs: AURORA-1532
> https://issues.apache.org/jira/browse/AURORA-1532
>
>
> Repository: aurora
>
>
> Description
> ---
>
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal. Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
>
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
>
> build-support/hooks/pre-commit
>| 3 +--
> build-support/jenkins/build.sh
>| 9 +
> build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}
>| 13 ++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/__init__.py}| 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/__init__.py} | 12
> +---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/BUILD} | 18
> +++---
> build-support/{python/checkstyle-check =>
> plugins/src/main/python/apache/aurora/pants/optional/__init__.py} | 12
> +---
>
> build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py
> | 36
> build-support/python/checkstyle
>| 34 --
> build-support/python/checkstyle-check
>| 6 +++---
> pants.ini
>| 43
> +++
> src/main/python/apache/aurora/admin/maintenance.py
>| 2 +-
> src/main/python/apache/aurora/client/api/__init__.py
>| 4 ++--
> src/main/python/apache/aurora/client/cli/client.py
>| 2 +-
> src/main/python/apache/aurora/client/cli/cron.py
>| 2 +-
> src/main/python/apache/thermos/core/process.py
>| 6 +++---
> src/main/python/apache/thermos/monitoring/process_collector_psutil.py
>| 1 -
> src/test/python/apache/aurora/admin/test_admin.py
>| 6 --
>
