-
Notifications
You must be signed in to change notification settings - Fork 4
Fill in {commit} for Travis PR builds by querying GitHub
#68
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
I confused: so this PR doesn't provide solution for deducing the corresponding
commitfor GH actions PR runs? For GH actions, like even in this PR we might have both "pull_request" (wherecommit != build_commit) and "push" (wherecommit == build_commitAFAIK) runs.Or it is just a matter of me not grasping it from the above description, that the comment above for "PR
pushbuilds" while "pull_request" builds will have differentcommitandbuild_commit?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.
For PR builds in GitHub Actions,
{commit}always equals{build_commit}, as the API reports the PR head commit as the commit being built (despite actually operating on a merge commit).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.
FTR my run on -inception: Running fetch for current state of this PR where
pull_requestrun has loggedHEAD is now at 03f35eb Merge c30b87e into 52c92e0
so the
{commit}is c30b87e and corresponding{build_commit}for a PR is a long version of 03f35ebwhich seems to be available within workflow under `GITHUB_SHA` env var and logged
using this tuned up config for inception
I got following output printed by tinuous
we indeed got correct "matching" c30b87e-c30b87e7 for push builds across all (good) + the one for github PR (not so good)
and travis and appveyor correctly had c30b87e-03f35ebb (`{commit}-{build-commit}`) for PRs
github actions got two separate logs (for PR and branch) logs and indeed
{build_commit}was corresponding to the actual{commit}:and actual `{build_comit}` (merge) was logged
With the observation of consistently logged
GITHUB_SHAwe could use that one for the case wherecommit == build_commitand take it instead for thebuild_commitif differs. Could we do that to overcome github deficiency?here is where I observe it being printed across all runs
so it seems to be nicely consistent.
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.
The
GITHUB_SHAline is part of the output from the Codecov action. It won't be present on workflows that don't use Codecov.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.
d'oh -- right. Could we (ab)use
as
line then may be, even though we would not get a full hexsha out of it (well, could be chased with a follow up search for a string starting from that since it seems to be right there:
I just do not want us to give up since then we can not nicely and easily group all runs across CIs and disambiguate between PR runs against different bases (e.g. if base is changed after PR is initiated) :-/
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.
and you did file a support request IIRC with github but no action was taken yet, right?
I also dislike "parse the logs" solution but poking around API did not find yet anything which could help -- there is no endpoint by any chance to query env variables for the run, is there (I did not find any)?
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.
No, I did not file a support request about this. Are you thinking of the support request I filed with Travis to include the PR head in their API?
I do not believe the GitHub API has an endpoint for workflow run variables.
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.
I guess I mixed up with that one indeed! Could you please file an issue with github so we possibly see if there is a bright future whenever it would be supported? I also wonder why they do not anyhow expose environment variables (e.g. even without secrets) -- that could have given us remedy
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.
Support issue filed.
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.
ok, let's hold breath for a little bit. I think we can proceed with this PR since for many usecases we can already have it working nicely, filed a dedicated #69 for the github actions.