GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix changed file detection on travis when the PR commits predate master commits. by ammaraskar · Pull Request #3129 · python/cpython

Conversation

ammaraskar commented Aug 18, 2017

Copy link

Copy Markdown

Member

See python/core-workflow#14 for my investigation into this.

I got the command for the pull request file detection from http://eng.localytics.com/best-practices-and-common-mistakes-with-travis-ci/ though there's is hardcoded to only work for master

CHANGES=$(git --no-pager diff --name-only FETCH_HEAD $(git merge-base FETCH_HEAD master))  
[ -n "$(grep '^rails' <<< "$CHANGES")" ] && testRails

additionally I found the exact same command here: https://stackoverflow.com/a/41159710
and here: https://stackoverflow.com/questions/25071579/list-all-files-changed-in-a-pull-request-in-git-github

I've also made the script print out what files changed so this is easier to debug if it becomes an issue again in the future.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

the-knights-who-say-ni added the CLA signed label

Aug 18, 2017

bedevere-bot added the awaiting review label

Aug 18, 2017

ammaraskar commented Aug 18, 2017

Copy link

Copy Markdown

Member Author

This can be labelled with both skip news and skip issue

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ammaraskar mentioned this pull request

Aug 18, 2017

Only run CI checks when appropriate files have changed python/core-workflow#14

Closed

brettcannon added skip issue skip news labels

Aug 18, 2017

brettcannon requested review from berkerpeksag and zware

August 18, 2017 18:50

orenmn mentioned this pull request

Sep 28, 2017

[2.7] bpo-31285: fix an assertion failure and a SystemError in warnings.warn_explicit (GH-3219) #3805

Closed

zware commented Jan 26, 2018

Copy link

Copy Markdown

Member

@ammaraskar Would you mind rebasing this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix changed file detection on travis when the PR commits predate mast…

df65aff

…er commits.

Travis when merging changes from a pull request onto the target branch
does not perform a rebase, instead it does a simple merge which causes
the PR commits to retain their commit dates. This means that the commit
log can potentially look like:

PR merge <-- HEAD
normal master commit <- master
more commits from normal workflow
PR commit 1
another master commit
PR commit 2

Performing a git diff from PR commit 2 to master will accidentally
include
files that should not be there.

Closes python/core-workflow#14

ammaraskar force-pushed the travis_fix branch from 35e176e to df65aff Compare

January 26, 2018 23:52

ammaraskar commented Jan 26, 2018

Copy link

Copy Markdown

Member Author

@zware done

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

berkerpeksag approved these changes

Jan 27, 2018

View reviewed changes

berkerpeksag left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry for my very late response. This looks good to me and I agree with Ammar's analysis at python/core-workflow#14 (comment). I just left two minor comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.travis.yml Outdated

		files_changed=$(git diff --name-only HEAD $(git merge-base HEAD $TRAVIS_BRANCH))
		fi

		echo "files_changed: "

berkerpeksag Jan 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you add a comment saying that this is for debugging purposes and rename "files_changed:" to "Files changed:" to follow the current style?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.travis.yml Outdated

		# Pull requests are slightly complicated because merging the PR commit without
		# rebasing causes it to retain its old commit date. Meaning in history if any
		# commits have been made on master that post-date it, they will be accidentally
		# included in the diff if we use the simple TRAVIS_COMMIT_RANGE method

berkerpeksag Jan 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can we just say "if we use TRAVIS_COMMIT_RANGE" or "if we use the TRAVIS_COMMIT_RANGE environment variable" to make it clearer? Perhaps it's just me but I got confused by the "method" part of that sentence :)

(Also, style nit: Can you finish the sentence with a full stop?)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting review labels

Jan 27, 2018


          Fix up comments in the travis file

eece657

ammaraskar commented Jan 27, 2018

Copy link

Copy Markdown

Member Author

Fixed up the comment style.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jayyyin commented Jan 27, 2018

Copy link

Copy Markdown

Contributor

everything looks good to me too, also tested the regex, this should be good to go.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix typo

ef6f8e8

berkerpeksag added the needs backport to 3.6 label

Jan 27, 2018

berkerpeksag merged commit b2ec361 into python:master

Jan 27, 2018

miss-islington commented Jan 27, 2018

Copy link

Copy Markdown

Contributor

Thanks @ammaraskar for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Jan 27, 2018

berkerpeksag commented Jan 27, 2018

Copy link

Copy Markdown

Member

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jan 27, 2018


          Fix changed file detection on Travis (pythonGH-3129)

130cd28

Travis when merging changes from a pull request onto the target branch
does not perform a rebase, instead it does a simple merge which causes
the PR commits to retain their commit dates. This means that the commit
log can potentially look like:

PR merge <-- HEAD
normal master commit <- master
more commits from normal workflow
PR commit 1
another master commit
PR commit 2

Performing a git diff from PR commit 2 to master will accidentally
include files that should not be there.

Closes python/core-workflowGH-14
(cherry picked from commit b2ec361)

bedevere-bot commented Jan 27, 2018

Copy link

Copy Markdown

GH-5365 is a backport of this pull request to the 3.6 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.6 label

Jan 27, 2018

berkerpeksag pushed a commit that referenced this pull request

Jan 27, 2018


          Fix changed file detection on Travis (GH-3129)

4d2dd64

Travis when merging changes from a pull request onto the target branch
does not perform a rebase, instead it does a simple merge which causes
the PR commits to retain their commit dates. This means that the commit
log can potentially look like:

PR merge <-- HEAD
normal master commit <- master
more commits from normal workflow
PR commit 1
another master commit
PR commit 2

Performing a git diff from PR commit 2 to master will accidentally
include files that should not be there.

Closes python/core-workflow#14

(cherry picked from commit b2ec361)

Reviewers

berkerpeksag

berkerpeksag approved these changes

zware

Awaiting requested review from zware

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.