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

Fix: Create Commit tree using GitHub API #321

Merged
merged 1 commit into from
May 5, 2024

Conversation

alvarezfr
Copy link
Contributor

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 and nunjucks because while running the src/ code locally with node v16.20.2 I was getting an error saying the function fs.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.

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
Copy link

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

Copy link
Contributor Author

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.

@skgsergio
Copy link

Just tested this and fixed my issue.

@svg153
Copy link
Contributor

svg153 commented Oct 2, 2023

@BetaHuhn this PR was tested in our org, by only this change:

image

Please, could you review it? Thanks :)

@alvarezfr
Copy link
Contributor Author

Hi @BetaHuhn.

Do you think you could review this PR? Thank you!

@kevinv-novibet
Copy link

@BetaHuhn when do you think we can review and merge this PR?

@mazilu88
Copy link

mazilu88 commented Feb 5, 2024

Hi @BetaHuhn,

Could you please review and merge this?

It solved our issue too Cannot create a new GitHub Tree: tree.sha XXXX is not a valid blob

Thanks!

Copy link
Owner

@BetaHuhn BetaHuhn left a 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.

@BetaHuhn BetaHuhn merged commit 899c636 into BetaHuhn:master May 5, 2024
@BetaHuhnBot
Copy link
Collaborator

🎉 This PR is included in version 1.21.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

8 participants