-
Notifications
You must be signed in to change notification settings - Fork 37
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
Action fails in pull on new data branch #33
Comments
Patched via #30. |
I am really sorry to disappoint, but I just tested the commit in the PR, and it doesn't solve this problem. Sorry, I should have actually tested it, instead of just assuming that the PR fixed this error. 😞 With the change, the pull still doesn't work.
@fboemer , was that actually the problem you wanted to solve and did your change fix it for you? I am not sure that the problem can be solved with a So I guess (ugh, guesswork again) if you want to keep the feature that a data branch can be created by a run, a different approach would be needed. |
Thanks for the report; I don't quite recall the error I was seeing (the related Actions artifacts have since expired) and I'm unfortunately out of Actions minutes for this month, so I can't reproduce for another week or so. @ajagann, do you have any insights here? |
@jgehrcke , you may want to reopen this issue. Sorry, I was too quick in assuming this problem was already fixed. The script does a |
In order to handle multiple jobs running in parallel, and thus pushing changes in parallel, a loop running pull and then push tries to get a job's changes pushed to the data repository. The problem here is that if the data branch was created locally as a new branch, then the pull before the push will fail and the whole script will exit. See issue jgehrcke#33. This is a very simple attempt to solve this issue by mimicing a developer's workflow, who tried to push and got rejected because of the remote having changes from another push. The lazy developer woud then pull and try to push again. This does look like overkill, to be honest, and is only a quick and dirty attempt at a solution. It is not very robust because it relies on the error output of `git push` which might change. Unfortunately, git commands do not have distinctive error codes that could be used instead of guessing from the error message.
We could also keep the order and simply ignore |
Well, you defined a maximum time to stay in the loop, so eventually it would error out at some point. The question is, what other errors could the |
Not so sure. What are all the expected failure modes, and which ones do we not want to 'ignore'? I mean, for example for the error discussed here in this issue where I'll think about this a tiny more soon. I think we're getting close. Thanks for all the valuable thinking here here! |
Yes and no, I would argue that in this case discussed the crash was not helpful as it would have remedied itself with the following (first) push. I'm not sure there is an elegant way. I didn't find a way for an "atomic" "check-for-branch-and-push" with the porcelain git commands. Right now I do a push first, then the pull and another push if the first push failed. That works, but in a scenario where the pull/push is necessary, i.e. where concurrent pushes occur, the first push is bound to fail. It works for me as I have it set up with a max parallel = 1 setting. |
pull/push loop: ignore pull errors (fix #33)
Accidental treatment of special cases is something I don't want to have in code :D But yes you're right, by now it's not accidental anymore. We're ignoring pull errors now: https://github.com/jgehrcke/github-repo-stats/pull/35/files#diff-6f9d41d046756f0ddc2fcee0626bdb50100d12b88f293734eff742818e03efa2L214 -- and I have noted the special case in the corresponding code comment. |
The action does not require the data branch to exist up front. This is good. If the data branch does not exist, it will be created.
But this breaks later in the script when a default
pull
is executed. The pull will have no tracked remote ref to operate on, since the branch is new. This results in the following error.I believe the PR #30 is there to address this problem, from the looks of it. I haven't tested it, though.
The text was updated successfully, but these errors were encountered: