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

Adding option to use private PAT token on GitHub Q&A sample #713

Merged
merged 53 commits into from
Jun 22, 2023

Conversation

mahomedalid
Copy link
Contributor

@mahomedalid mahomedalid commented Apr 28, 2023

Motivation and Context

  1. Why is this change required? What problem does it solve? What scenario does it contribute to?

Most integrations of OpenAPI and Semantic Kernel would have the motivation to integrate it with private information or non-public data. GitHub Q&A is an amazing sample, and be able to try it with private repositories it is a very straightforward way to solve scenarios where customers and users want to try it with private data.

Fixes #444

Description

  • Added a new Input in the React App, this is a password type of input. I decided to not include this pat token as part of the verification if it is a different repo.
  • In the GitHub skill I added the PatToken parameter using the existing context variables pattern. I saw this as straightforward since the PatToken may be used for other GitHub skill features.
  • WebFileDownloadSkill is modified to receive any additional headers to the request. I see that WebFileDownloadSkill could have had 3 ways to modify the headers of the request: 1) modify the constructor and class to receive and/or make HttpClient public, 2) Add the headers as part of the context variables received, 3) Override the download methods available to have one that specifically receive custom headers. I decided to use 3 to have the minimal impact in existing code (and tests), although I could see 2 could also be a good approach. Then the GitHub skill detects that an authorization header is necessary, and calls the specific overrided method.

Pending

  • Add Unit and/or integration tests.

Contribution Checklist

@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel samples labels Apr 28, 2023
@mahomedalid mahomedalid requested a review from shawncal May 8, 2023 21:09
@hathind-ms hathind-ms requested review from a team as code owners June 19, 2023 22:22
@github-actions github-actions bot removed the kernel Issues or pull requests impacting the core kernel label Jun 21, 2023
@mahomedalid
Copy link
Contributor Author

@adrianwyatt @craigomatic I saw a change made from the call to an older github api to the latest one. The latest one changes two things: now it redirects to a temporary (5min) private link (302), the redirect request will also need the authorization headers, and it requires extra headers (X-GitHub-Api-Version, User-Agent, Accept), which were not there. Updated, and should be back to working state now.

@craigomatic craigomatic added PR: ready for review All feedback addressed, ready for reviews and removed PR: paused PR temporarily parked labels Jun 21, 2023
Copy link
Contributor

@craigomatic craigomatic left a comment

Choose a reason for hiding this comment

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

Validated that this works using a fine-grained token with Contents = read only on a private repo, ie:
image

If you can make the 1 change discussed offline I'm ok bringing this one in.

@craigomatic craigomatic added PR: feedback to address Waiting for PR owner to address comments/questions and removed PR: ready for review All feedback addressed, ready for reviews labels Jun 21, 2023
@craigomatic craigomatic added PR: ready to merge PR has been approved by all reviewers, and is ready to merge. and removed PR: feedback to address Waiting for PR owner to address comments/questions labels Jun 21, 2023
@adrianwyatt adrianwyatt added this pull request to the merge queue Jun 22, 2023
Merged via the queue into microsoft:main with commit f17a0a1 Jun 22, 2023
16 checks passed
shawncal added a commit to shawncal/semantic-kernel that referenced this pull request Jul 6, 2023
…t#713)

### Motivation and Context

1. Why is this change required? What problem does it solve? What
scenario does it contribute to?

Most integrations of OpenAPI and Semantic Kernel would have the
motivation to integrate it with private information or non-public data.
GitHub Q&A is an amazing sample, and be able to try it with private
repositories it is a very straightforward way to solve scenarios where
customers and users want to try it with private data.

Fixes microsoft#444 

### Description

* Added a new Input in the React App, this is a password type of input.
I decided to not include this pat token as part of the verification if
it is a different repo.
* In the GitHub skill I added the PatToken parameter using the existing
context variables pattern. I saw this as straightforward since the
PatToken may be used for other GitHub skill features.
* WebFileDownloadSkill is modified to receive any additional headers to
the request. I see that WebFileDownloadSkill could have had 3 ways to
modify the headers of the request: 1) modify the constructor and class
to receive and/or make HttpClient public, 2) Add the headers as part of
the context variables received, 3) Override the download methods
available to have one that specifically receive custom headers. I
decided to use 3 to have the minimal impact in existing code (and
tests), although I could see 2 could also be a good approach. Then the
GitHub skill detects that an authorization header is necessary, and
calls the specific overrided method.

### Pending

* Add Unit and/or integration tests.

### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [X] The code builds clean without any errors or warnings
- [X] The PR follows SK Contribution Guidelines
(https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [X] The code follows the .NET coding conventions
(https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions)
verified with `dotnet format`
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Shawn Callegari <36091529+shawncal@users.noreply.github.com>
Co-authored-by: Lee Miller <lemiller@microsoft.com>
Co-authored-by: Adrian Bonar <56417140+adrianwyatt@users.noreply.github.com>
Co-authored-by: Harleen Thind <39630244+hathind-ms@users.noreply.github.com>
Co-authored-by: Craig Presti <146438+craigomatic@users.noreply.github.com>
Co-authored-by: Adrian Bonar <adribona@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code PR: ready to merge PR has been approved by all reviewers, and is ready to merge.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Private repos on the GitHub Q&A sample
10 participants