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

Add support for signed commits through the Github API #492

Draft
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

ChrisStatham
Copy link

@ChrisStatham ChrisStatham commented Jul 17, 2024

What does this change

Give a summary of the change, and how it affects end-users. It's okay to copy/paste your commit messages.

This PR adds the ability to commit through the Github API, rather than through the git client. This creates a signed commit for both user accounts, and for Github Apps.

For example if it introduces a new flag or modifies a commands output, give an example of you running the command and showing real output here.

INFO[0000] Running on 1 repositories                    
INFO[0000] Cloning and running script                    repo=scorebet/managed-actions
INFO[0004] Pushing changes to remote                     repo=scorebet/managed-actions
INFO[0005] Creating pull request                         repo=scorebet/managed-actions
Repositories with a successful run:
  scorebet/managed-actions #26

What issue does it fix

Closes #261

If there is not an existing issue, please make sure we have context on why this change is needed. .

Notes for the reviewer

Put any questions or notes for the reviewer here.

I'm checking to see if this PR is sane. I'm not sure if commit signing is available using a Github User, but Github Applications need to commit though the Github API in order to get signed commits. There is a fairly decent write up for this issue here

Committing through the API is guaranteed to create a signed commit, so it removes all ambiguity

Checklist

  • Made sure the PR follows the CONTRIBUTING.md guidelines
  • Tests if something new is added ( I will happily add test cases if you are okay with the general approach)

@ChaosCypher
Copy link
Contributor

🙌

@ChrisStatham ChrisStatham marked this pull request as ready for review July 23, 2024 13:45
@ChrisStatham ChrisStatham marked this pull request as draft July 23, 2024 13:46
@ChrisStatham
Copy link
Author

Hey @lindell I'm hoping to get a review on this code. Just wondering if you find it acceptable. I'm happy to add unit tests, if needed.

@lindell
Copy link
Owner

lindell commented Jul 28, 2024

I think adding API committing is reasonable.
I've looked through the PR, and the general feedback on the structure is as follows:

  • The flag/setting should be called something not specific to any platform. Something like api-commit, commit-with-api or similar.
  • CMD-Git have a no-op implementation, but did not see any comments on it. What will the status be there?
  • If possible, I think it would be nice to get the changes/deletions when it is needed (when the push is suppose to happen, by looking at what was committed). Mainly to reduce state in the git implementation.
  • The base64 encoding of the data is very much as GitHub API implementation, and should therefore live in the github package. The git package could simply return the raw data per file changed. This would also make it easier to implement this feature for other platforms.

I have not looked super close into all line of code, and do not want to do so at this point as things might still change. But something I found in at least two places where manual path handling through string manipulation. Please do use the path or the path/filepath in these cases if you edit them.

Thanks for the contribution, looking forward to merging this 😄

@ChaosCypher
Copy link
Contributor

+1 on this feature!

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.

multi-gitter run does not sign commits
3 participants