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.

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

Skip push builds if there is a pull request #5156

Merged
merged 6 commits into from
Sep 13, 2018
Merged

Conversation

lbergelson
Copy link
Member

testing if this skips the push builds.

@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

Merging #5156 into master will decrease coverage by 0.054%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##              master    #5156       +/-   ##
==============================================
- Coverage     86.714%   86.66%   -0.054%     
- Complexity     29399    29413       +14     
==============================================
  Files           1815     1813        -2     
  Lines         136044   136388      +344     
  Branches       15091    15028       -63     
==============================================
+ Hits          117969   118194      +225     
- Misses         12644    12749      +105     
- Partials        5431     5445       +14
Impacted Files Coverage Δ Complexity Δ
.../utils/read/markduplicates/LibraryIdGenerator.java 57.895% <0%> (-31.579%) 7% <0%> (-7%)
...e/hellbender/cmdline/PicardCommandLineProgram.java 53.846% <0%> (-23.077%) 2% <0%> (-1%)
...institute/hellbender/utils/io/IOUtilsUnitTest.java 78.81% <0%> (-8.909%) 42% <0%> (+10%)
...ls/read/markduplicates/GATKDuplicationMetrics.java 93.103% <0%> (-6.897%) 12% <0%> (-1%)
...ender/tools/spark/sv/utils/SVIntervalUnitTest.java 91.176% <0%> (-4.476%) 5% <0%> (+1%)
...ypecaller/AssemblyBasedCallerGenotypingEngine.java 88.018% <0%> (-3.497%) 87% <0%> (+17%)
.../AbstractMarkDuplicatesCommandLineProgramTest.java 96.296% <0%> (-3.488%) 88% <0%> (+6%)
...rg/broadinstitute/hellbender/utils/io/IOUtils.java 68.881% <0%> (-2.133%) 70% <0%> (+16%)
...er/tools/funcotator/FuncotatorIntegrationTest.java 83.473% <0%> (-1.241%) 79% <0%> (ø)
...roadinstitute/hellbender/utils/read/ReadUtils.java 79.72% <0%> (-0.932%) 203% <0%> (-6%)
... and 53 more

@lbergelson lbergelson changed the title testing skipping push builds on pr DO NOT MERGE Skip push builds if there is a pull request Sep 5, 2018
@lbergelson
Copy link
Member Author

@droazen I implemented the pr skipping on push builds if there's a pr branch. It seems to work. It has to spin up a vm to do the check, but that takes about a minute instead of many, and it avoids running tests and downloading lfs. The good things is that if it fails for some reason it should just continue on with the build, so flakiness in the github api or network connectivitiy will just result in some extra builds completing rather than extra failures.

screen shot 2018-09-05 at 11 19 14 am

I think I should add a github token though so we don't get api throttling. Should I just add one from my own account?

… ever leave you'll probably have to update it.
@lbergelson
Copy link
Member Author

testing

Copy link
Collaborator

@droazen droazen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete, back to @lbergelson. Merge after addressing comments.

.travis.yml Outdated
- if [[ ${TRAVIS_EVENT_TYPE} == push ]];
then
PULL_REQUESTS=$(curl -v -H "Authorization':' token $GITHUB_API_TOKEN" https://api.github.com/repos/broadinstitute/gatk/pulls\?state\=open\&head="broadinstitute:${TRAVIS_BRANCH}");
if [[ $( grep -c "commits" <<< ${PULL_REQUESTS} ) -gt 0 ]];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of grepping for a specific term, perhaps it would be better to check that the result is non-empty after trimming [ ]. Eg., check that:

curl https://api.github.com/repos/broadinstitute/gatk/pulls\?state\=open\&head="broadinstitute:lb_skip_push_buildsfoo" | sed 's/\[//g' | sed 's/\]//g' | tr -d '[:space:]' | wc -c

is non-zero.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to skip this because I'm not convinced it's any more robust to changes in the returned format.

.travis.yml Outdated
#skip push builds if there's an associated PR because we don't look at them anway
- if [[ ${TRAVIS_EVENT_TYPE} == push ]];
then
PULL_REQUESTS=$(curl -v -H "Authorization':' token $GITHUB_API_TOKEN" https://api.github.com/repos/broadinstitute/gatk/pulls\?state\=open\&head="broadinstitute:${TRAVIS_BRANCH}");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a separate account tied to this token instead of using your personal account?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFFIRMATIVE

@droazen droazen assigned lbergelson and unassigned droazen Sep 10, 2018
@gatk-bot
Copy link

Does not compute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants