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

More fixes for git baseline selection in pr-check #20216

Conversation

danielrozenberg
Copy link
Member

Turns out that TRAVIS_COMMIT_RANGE is frozen at the point of the PR's creation, not when the PR check is actually running.

danielrozenberg pushed a commit to danielrozenberg/amphtml that referenced this pull request Jan 9, 2019
@danielrozenberg danielrozenberg force-pushed the travis-test-local-changes-baseline branch from 92d5afb to 806578f Compare January 9, 2019 22:31
@danielrozenberg danielrozenberg force-pushed the travis-test-local-changes-baseline branch from a0b6ab6 to 36055de Compare January 9, 2019 23:00
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

I've requested 2 cosmetic changes, and made a few sanity check comments. I'll approve once you ack / resolve all of them.

build-system/pr-check.js Show resolved Hide resolved
build-system/pr-check.js Outdated Show resolved Hide resolved
build-system/git.js Show resolved Hide resolved
build-system/git.js Show resolved Hide resolved
build-system/git.js Show resolved Hide resolved
build-system/git.js Show resolved Hide resolved
build-system/git.js Show resolved Hide resolved
build-system/git.js Show resolved Hide resolved
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM once the logging changes are made.

@danielrozenberg
Copy link
Member Author

danielrozenberg commented Jan 9, 2019

For posterity, here are the tests for the logic:

In all cases, the values returned by the modified function was the one we wanted

@rsimha
Copy link
Contributor

rsimha commented Jan 9, 2019

Nice work with all the checks in #20216 (comment).

One last corner case to verify: Reopen #20231 after one or more new commits have been merged to upstream/master and make sure the baseline moves forward.

@danielrozenberg
Copy link
Member Author

Done. Updated #20216 (comment)

@danielrozenberg danielrozenberg deleted the travis-test-local-changes-baseline branch January 10, 2019 00:05
@danielrozenberg danielrozenberg restored the travis-test-local-changes-baseline branch January 10, 2019 00:06
@danielrozenberg danielrozenberg merged commit b03c6fd into ampproject:master Jan 10, 2019
@danielrozenberg danielrozenberg deleted the travis-test-local-changes-baseline branch January 10, 2019 00:13
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* More fixes for git baseline selection in pr-check

* Fix typo

* Use `master`, not `origin/master`, when choosing merge-base

* Also print TRAVIS_COMMIT

* Use TRAVIS_PULL_REQUEST_SHA instead of TRAVIS_COMMIT

* Do not print TRAVIS_COMMIT

* Address Raghu's comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants