-
Notifications
You must be signed in to change notification settings - Fork 6.8k
build: get previous payload by querying git #5122
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
Conversation
c25888f
to
26bfc91
Compare
tools/gulp/tasks/payload.ts
Outdated
function getCommitFromPreviousPayloadUpload(): string { | ||
if (isTravisMasterBuild()) { | ||
const commitRange = process.env['TRAVIS_COMMIT_RANGE']; | ||
const commitCount = spawnSync('git', ['rev-list', '--count', commitRange]).stdout |
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 comments that write out the commit commands and describe what you're getting from them?
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.
Added comments. It was pretty hard to explain what those Git calls do. Please me know if you think something needs to be better explained.
72925dc
to
8a06781
Compare
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.
LGTM
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.
Grammar nits
tools/gulp/tasks/payload.ts
Outdated
function getCommitFromPreviousPayloadUpload(): string { | ||
if (isTravisMasterBuild()) { | ||
const commitRange = process.env['TRAVIS_COMMIT_RANGE']; | ||
// In some situations Travis will include multiple commits in a single Travis Job. This means |
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.
In some situations Travis
-> In some situations, Travis
tools/gulp/tasks/payload.ts
Outdated
const commitRange = process.env['TRAVIS_COMMIT_RANGE']; | ||
// In some situations Travis will include multiple commits in a single Travis Job. This means | ||
// that we can't just resolve the previous commit by using the parent commit of HEAD. | ||
// By resolving the amount of commits inside of the current Travis Job we can figure out |
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.
By resolving the amount of commits inside of the current Travis Job we can
-> By resolving the amount of commits inside of the current Travis job, we can
tools/gulp/tasks/payload.ts
Outdated
// how many commits before HEAD the last Travis Job ran. | ||
const commitCount = spawnSync('git', ['rev-list', '--count', commitRange]).stdout | ||
.toString().trim(); | ||
// With the amount of commits inside of the current Travis Job we can query Git to print |
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.
With the amount of commits inside of the current Travis job we can
-> With the amount of commits inside of the current Travis job, we can
tools/gulp/tasks/payload.ts
Outdated
return spawnSync('git', ['rev-parse', `HEAD~${commitCount}`]).stdout.toString().trim(); | ||
} else { | ||
// Travis applies the changes of Pull Requests in new branches. This means that resolving | ||
// the commit, that previously ran on the target branch (mostly "master"), can be done |
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.
This means that resolving the commit, that previously
-> This means that resolving the commit that previously
Currently the previous payload is just the latest uploaded payload result on Firebase. This is problematic because it can happen that the current payload is uploaded "too fast" and the payload task is comparing against itself. Just calculating before uploading could work most of the time, but there it would still fail if another Travis Job uploades payload results at the same time. The safest solution would be resolving the parent commit of the current Travis job. This way the payload diff would be always correct (even if multiple Jobs are running).
8a06781
to
cc56402
Compare
@kara Fixed all grammar comments. Thanks for those! |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently the previous payload is just the latest uploaded payload result on Firebase. This is problematic because it can happen that the current payload is uploaded "too fast" and the payload task is comparing against itself.
Just calculating before uploading could work most of the time, but there it would still fail if another Travis Job uploades payload results at the same time.
The safest solution would be resolving the parent commit of the current Travis job. This way the payload diff would be always correct (even if multiple Jobs are running).