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

GitHub upload using GraphQL #882

Merged
merged 51 commits into from
Jun 22, 2022
Merged

GitHub upload using GraphQL #882

merged 51 commits into from
Jun 22, 2022

Conversation

samchungy
Copy link
Contributor

@samchungy samchungy commented May 29, 2022

Adds new methods to use the GitHub GraphQL API to push code. This signs our commits which resolves #684

Each call seems to consume only 1 point per call out of our 5000 points/hour quota.

@changeset-bot
Copy link

changeset-bot bot commented May 29, 2022

🦋 Changeset detected

Latest commit: 4104f25

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@samchungy samchungy added the dino:snooze Snooze in Review Dino label May 29, 2022
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/api/github/push.ts Show resolved Hide resolved
Comment on lines 91 to 93
contents: await fs.readFile(filePath, {
encoding: 'base64',
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a request size limit that we need to be aware of here? In that scenario, does GitHub provide an option to upload files separately? (I'm hoping we can avoid this complexity, but just thinking out loud.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None that they publicly say. Does not look like there's an option to upload files separately unless you want to use their REST API which requires about 3 different API calls to make a commit :\

@samchungy samchungy marked this pull request as ready for review June 4, 2022 11:49
@samchungy samchungy requested review from a team as code owners June 4, 2022 11:49
@samchungy samchungy removed the dino:snooze Snooze in Review Dino label Jun 4, 2022
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the 🐌 review! Mostly doco nitpicks that I'll apply in a sec.

.changeset/unlucky-kings-provide.md Outdated Show resolved Hide resolved
docs/development-api/git.md Outdated Show resolved Hide resolved
src/api/github/push.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/cli/lint/autofix.ts Outdated Show resolved Hide resolved
docs/development-api/github.md Outdated Show resolved Hide resolved
docs/development-api/github.md Outdated Show resolved Hide resolved
@@ -119,5 +165,23 @@ await GitHub.putIssueComment({

---

## mapChangedFilesToFileChanges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is an I/O operation, I wonder if the verb should here be read or load?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that makes sense, when I first started this I thought it was going to be a simple map.

docs/development-api/github.md Outdated Show resolved Hide resolved
docs/development-api/github.md Outdated Show resolved Hide resolved
src/api/git/pull.ts Outdated Show resolved Hide resolved
src/api/git/pull.ts Outdated Show resolved Hide resolved
src/api/git/pull.ts Outdated Show resolved Hide resolved
src/api/github/push.ts Outdated Show resolved Hide resolved
.changeset/thin-carrots-burn.md Outdated Show resolved Hide resolved

These file changes will appear as verified commits on GitHub.

Please note: this will not update the local Git repository
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Please note: this will not update the local Git repository
This function is roughly equivalent to `git push`,
but it will not update the local Git repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest it's not similar since push would push commits and this would not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps maybe avoiding push might be wise? Would upload make more sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I clearly wrote the wrong thing here. Will revert 🙇

docs/development-api/github.md Outdated Show resolved Hide resolved
docs/development-api/github.md Outdated Show resolved Hide resolved
src/api/github/push.ts Outdated Show resolved Hide resolved
@72636c 72636c enabled auto-merge (squash) June 22, 2022 07:45
@72636c 72636c disabled auto-merge June 22, 2022 07:48
@72636c 72636c changed the title GitHub Push using GraphQL GitHub upload using GraphQL Jun 22, 2022
@72636c 72636c enabled auto-merge (squash) June 22, 2022 10:22
@72636c 72636c merged commit c7066cb into master Jun 22, 2022
@72636c 72636c deleted the graphql-commit branch June 22, 2022 10:24
@seek-oss-ci seek-oss-ci mentioned this pull request Jun 22, 2022
72636c added a commit that referenced this pull request Jun 22, 2022
72636c added a commit that referenced this pull request Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Commit Signing
2 participants