-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fix: Create Commit tree using GitHub API #321
Conversation
e6e07f5
to
33b1a11
Compare
e2fe115
to
0fad1b1
Compare
false | ||
) | ||
|
||
// Creates the blob. We don't need to store the response because the local sha is the same and we can use it to reference the blob |
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.
Hi,
We have integrated this PR as it solves a couple of issues for us, however - while I can't find documentation on it - we have observed a behavior (that happens rarely) where the local sha is different from the sha returned by github api. I know it makes no sense given git internals, not sure what exactly happens here, probably some transformation on the content/filename by the API.
So I would amend the code to use the remote sha
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.
Hi @sthuck.
When did you find the problem? I found the same problem that you described after publishing the pull request, but I solved it (or I thought I did at least) the day 11/09. I forced pushed the commit to my branch replacing the previous commit.
The problem was caused by my usage of the ExecCmd function which by default trims the output of the command. In this case, I use it to get the content of a blob in the local git, which is a problem for the files is a problem for files which end with empty lines. I solved it passing the parameter to disable the trimming of the command (alvarezfr@0fad1b1#diff-30246b15f18bb39311579208583260447efd5ee017c25172393ad4505bb8c5a4R292). Based on my tests this solves the issue.
Just tested this and fixed my issue. |
@BetaHuhn this PR was tested in our org, by only this change: Please, could you review it? Thanks :) |
Hi @BetaHuhn. Do you think you could review this PR? Thank you! |
@BetaHuhn when do you think we can review and merge this PR? |
Hi @BetaHuhn, Could you please review and merge this? It solved our issue too Thanks! |
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.
Thanks @alvarezfr for taking the time to look into the issues and creating the PR!
Also sorry everyone for my long absence and lack of action on this PR, finally found some time to take a look today.
With my limited testing everything still seems to work, although I haven't tested it with a GH_INSTALLATION_TOKEN
specifically so I am trusting everybody's tests that this works as well.
Please let me know if anyone runs into issues after this is merged.
🎉 This PR is included in version 1.21.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Changes the way that we are creating GitHub trees, because we are hitting some problems on the GitHub side receiving 502 status code with no more information than "Server Error" when creating the tree using the API, and this kind of error is not documented in GitHub but I was able to found some other repositories referencing to this.
This PR changes the way that we create a git tree in GitHub. Now, instead of creating the full tree to commit, we create a new tree with only the changes and references to the git tree of the previous commit in the Create Tree API.
I found the issue #246 after getting the same error in some syncs. The code has been tested with my own repositories with this kind of error.
Also, why I changed some imports:
I have changed the imports of
fs-extra
andnunjucks
because while running the src/ code locally with node v16.20.2 I was getting an error saying the functionfs.existsSync
doesn't exist, but it was working just fine if I run the packaged dist version. After looking at the documentation, I changed it to the recommended.But after making the previous change, I was receiving an error with the nunjuncks which said the function
nunjucks.configure
did not exist.