Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUILD] refactor dev/lint-python in to something readable #22994

Closed
wants to merge 1 commit into from
Closed

[BUILD] refactor dev/lint-python in to something readable #22994

wants to merge 1 commit into from

Conversation

shaneknapp
Copy link
Contributor

@shaneknapp shaneknapp commented Nov 9, 2018 •
edited

What changes were proposed in this pull request?

dev/lint-python is a mess of nearly unreadable bash. i would like to fix that as best as i can.

How was this patch tested?

the build system will test this.

@shaneknapp shaneknapp changed the title this is serious refactor [BUILD] refactor dev/lint-python in to something readable Nov 9, 2018
@shaneknapp shaneknapp mentioned this pull request Nov 9, 2018
Closed
@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98656 has finished for PR 22994 at commit 8fd68aa.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98657 has started for PR 22994 at commit 86f7707.

@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98658 has started for PR 22994 at commit 6acba48.

@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98659 has finished for PR 22994 at commit 58bf593.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98661 has finished for PR 22994 at commit 308d237.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98662 has started for PR 22994 at commit 308d237.

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98664 has started for PR 22994 at commit 7d366d2.

@shaneknapp
Copy link
Contributor Author

see the output from the following build to get the gory details of what's happening:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98664/console

i removed set -x from the top to clean up the output in the build logs.

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98665 has started for PR 22994 at commit 2148f61.

@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98666 has finished for PR 22994 at commit bd95fbc.

  • This patch fails executing the dev/run-tests script.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98667 has started for PR 22994 at commit 6bddfec.

@SparkQA
Copy link

SparkQA commented Nov 9, 2018

Test build #98668 has started for PR 22994 at commit c05683b.

@SparkQA
Copy link

SparkQA commented Nov 10, 2018

Test build #98669 has finished for PR 22994 at commit 56329bc.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 10, 2018

Test build #98673 has finished for PR 22994 at commit 56329bc.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Nov 10, 2018 •
edited

I agree with this change in general. The current script is a total mess - I will try to help take a look when the tests pass. BTW, it would be awesome if PR description contains what this PR tries to fix later when the tests pass

@shaneknapp
Copy link
Contributor Author

i have no idea why the tests aren't passing btw. :\

@shaneknapp
Copy link
Contributor Author

test this please

1 similar comment
@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 11, 2018

Test build #98687 has finished for PR 22994 at commit 56329bc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 11, 2018

Test build #98688 has finished for PR 22994 at commit 1f5c2f5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shaneknapp
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Nov 12, 2018

Test build #98733 has finished for PR 22994 at commit 1f5c2f5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shaneknapp
Copy link
Contributor Author

shaneknapp commented Nov 13, 2018 •
edited

alright, i think we're g2g. i'll squash my commits now before this is merged.

@shaneknapp
parent 6cd2348
2a3a0e1 
author shane knapp <incomplete@gmail.com> 1541791117 -0800
committer shane knapp <incomplete@gmail.com> 1542135008 -0800

parent 6cd2348
author shane knapp <incomplete@gmail.com> 1541791117 -0800
committer shane knapp <incomplete@gmail.com> 1542134986 -0800

parent 6cd2348
author shane knapp <incomplete@gmail.com> 1541791117 -0800
committer shane knapp <incomplete@gmail.com> 1542134619 -0800

this is serious refactor

add tracing

fixing some problems

print out a happy success message

add underscore to flake8 executable definition

adding quotes around passed variable to properly test

adding more quotes around passed variable to properly test

even more quotes

i know which python we use now, removing debugging

adding spaces around shell escapes

apparently quoting these doenst work as advertised

final round of cleanup and formatting

removing set -x

output readability

more output cleanup

more more output cleanup

add tracing

add underscore to flake8 executable definition

adding quotes around passed variable to properly test

even more quotes

i know which python we use now, removing debugging

apparently quoting these doenst work as advertised

final round of cleanup and formatting

removing set -x

output readability

extra space removed

more output cleanup

more more output cleanup
@SparkQA
Copy link

SparkQA commented Nov 13, 2018

Test build #98790 has finished for PR 22994 at commit 2a3a0e1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shaneknapp
Copy link
Contributor Author

(weird, github ate my last comment)

@HyukjinKwon i think we're g2g for merging this in to master and backports. you want to do this, or should i?

@HyukjinKwon
Copy link
Member

I haven't taken a look super closely but the idea looks itself okay. Is it urgent? if yes, yup. I don't object to go ahead right away. Otherwise, might be good to leave open for few days for review comments ..

Let me leave some cc's for @srowen, @felixcheung, @holdenk ..

@shaneknapp
Copy link
Contributor Author

shaneknapp commented Nov 14, 2018 via email

@shaneknapp
Copy link
Contributor Author

howdy howdy! i opened this nearly 2 weeks ago, and was wondering if i could get another set of eyeballs on it...

@holdenk @srowen @felixcheung

@srowen
Copy link
Member

srowen commented Nov 20, 2018

I don't know this script well enough nor Python to really comment, but if it works, push it.

@shaneknapp
Copy link
Contributor Author

okie dokie... this will be my first official push to the spark repo! :)

@asfgit asfgit closed this in 42c4838 Nov 20, 2018
@shaneknapp
Copy link
Contributor Author

gonna hold off on backporting until i inspect each branch independently.

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@shaneknapp @jackylee-ch
[BUILD] refactor dev/lint-python in to something readable
7ced010 
## What changes were proposed in this pull request?

`dev/lint-python` is a mess of nearly unreadable bash.  i would like to fix that as best as i can.

## How was this patch tested?

the build system will test this.

Closes apache#22994 from shaneknapp/lint-python-refactor.

Authored-by: shane knapp <incomplete@gmail.com>
Signed-off-by: shane knapp <incomplete@gmail.com>
@shaneknapp shaneknapp deleted the lint-python-refactor branch April 19, 2019 17:35
@HyukjinKwon
Copy link
Member

This was merged to master (and branch-3.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@shaneknapp @SparkQA @HyukjinKwon @srowen