search

kubernetes / git-sync

A sidecar app which clones a git repo and keeps it in sync with the upstream.
Apache License 2.0
2.13k stars 406 forks source link

Add support for GitHub app authentication #878

Open risset opened 1 month ago

risset commented 1 month ago

Related issue: https://github.com/kubernetes/git-sync/issues/877

Currently the e2e::github_app_auth test should work locally, if the correct env vars are set. I've tested the feature that way before creating the PR. Ideally it would be possible to run it in the GHA workflow. One way I thought that might be feasible is if an app was created and installed to a particular (private) repo, and then its private key was stored as a git-sync repository secret.

Some information about GitHub app authentication: https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app

linux-foundation-easycla[bot] commented 1 month ago

CLA Signed

The committers listed above are authorized under a signed CLA.

k8s-ci-robot commented 1 month ago

Welcome @risset!

It looks like this is your first PR to kubernetes/git-sync 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/git-sync has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot commented 1 month ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: risset Once this PR has been reviewed and has the lgtm label, please ask for approval from thockin. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes/git-sync/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
nan-yu commented 1 month ago

While setting up a Github app, I noticed instead of using App ID to get installation tokens, it also supports using the Client ID.

I'm wondering have you experimented with the client ID?

risset commented 3 weeks ago

While setting up a Github app, I noticed instead of using App ID to get installation tokens, it also supports using the Client ID.

I'm wondering have you experimented with the client ID?

I didn't know about this, thanks. It appears to be a recent feature: https://github.blog/changelog/2024-05-01-github-apps-can-now-use-the-client-id-to-fetch-installation-tokens/

So the App ID and Client ID can be used interchangeably in this case.

The application ID is not being deprecated at this time, nor are their plans to remove it. However, compatibility with future features will rely on use of the client ID, so updating is recommended.

Should we have both the app ID and client ID as parameters and accept either of them, or just replace app ID with client ID?

nan-yu commented 3 weeks ago

Should we have both the app ID and client ID as parameters and accept either of them, or just replace app ID with client ID?

I would vote for supporting both scenarios.

nan-yu commented 1 week ago

Hi @risset , any recent update on addressing the comments in this PR?

I'm working on Config Sync. If possible, we would like to leverage this feature in our next minor release in July.

thockin commented 1 week ago

Hi Nan,

Before we can merge this we need to make sure we can test it properly, which involves a private key. I have not had the time to play and see how private it really needs to be, or if maybe we can get it to test via GH actions.

If you have time, that's where we need to spend it.

On Mon, Jun 24, 2024, 9:38 PM Nan Yu @.***> wrote:

Hi @risset https://github.com/risset , any recent update on addressing the comments in this PR?

I'm working on Config Sync https://cloud.google.com/kubernetes-engine/enterprise/config-sync/docs/overview. If possible, we would like to leverage this feature in our next minor release in July.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/git-sync/pull/878#issuecomment-2187960417, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKWAVAJOQGHD6LXYS6QTZDZJDX5HAVCNFSM6AAAAABH6TO2F6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXHE3DANBRG4 . You are receiving this because you were assigned.Message ID: @.***>

risset commented 1 week ago

Hi @risset , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

nan-yu commented 3 days ago

Hi Liam Wyllie , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

Thanks for the update. Did you run the e2e test with a public repo or a private repo? I followed the instruction with a private repo, and git-sync returned a repo not found error. Not sure if that works for you.

risset commented 1 day ago

Hi Liam Wyllie , any recent update on addressing the comments in this PR?

Hi, I will be working on this some more over the next few days! I've been having some issues with running test_e2e.sh on my current (macOS) machine, but I'm able to test my changes on another Linux machine at least.

Thanks for the update. Did you run the e2e test with a public repo or a private repo? I followed the instruction with a private repo, and git-sync returned a repo not found error. Not sure if that works for you.

I've been testing with a private repo. I'll double check the instructions and how I've set the app up etc. in case I missed something