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.

Sign up for GitHub

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

Jump to bottom

feat: create new GitHub Authentication Provider extension #5400

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

feat: create new GitHub Authentication Provider extension #5400

wants to merge 16 commits into from
+538 −1

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Jan 2, 2024 •
edited

What does this PR do?

Adding a GitHub Authentication Provider Extension to fix the rate limit when using GitHub api.

Screenshot / video of UI

Screenshots

preference configuration
image
sign in
image
LOGGED IN
image

configured invalid token on startup
image

What issues does this PR fix or reference?

Closing #4079

How to test this PR?

  • Create an access token
  • Put the access token in the properties
  • Sign in through the Authentication page

@axel7083
feat: create new GitHub Authentication Provider extension
b38f734 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 axel7083 requested review from benoitf and a team as code owners January 2, 2024 12:49
@axel7083 axel7083 requested review from dgolovin and cdrage and removed request for a team January 2, 2024 12:49
@axel7083
docs: improve the property description for GitHub extension
e494569 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083
test: adding unit tests
296c0e3 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083
test: more unit tests
4ea033c 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083
fix: removing todo comment
e46ebad 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083
Copy link
Contributor Author

axel7083 commented Jan 2, 2024

@benoitf The SonarCloud is giving an error for the build.js script located in the build folder for the extension created, it is a copy/past of the script used in the other extension, any clue ?

@axel7083
feat: adding telemetry usage
0ccea35 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@benoitf
Copy link
Collaborator

benoitf commented Jan 2, 2024

@axel7083 we need to review in SonarCloud UI the issues and we can flag them as 'safe' (which is the case there)

I've applied the change

lstocchi
lstocchi approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM

extensions/compose/src/utils.ts Outdated Show resolved Hide resolved
extensions/kind/src/util.ts Outdated Show resolved Hide resolved
@axel7083
fix: remove console.log
284e643 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 axel7083 mentioned this pull request Jan 4, 2024
Merged
3 tasks
dgolovin
dgolovin requested changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@dgolovin dgolovin left a comment

Choose a reason for hiding this comment

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

  1. There is not need for global var to store disposable, just use extensionContext.subscriptions
  2. Refactoring for king and compose should be in additional PR.

extensions/github/src/extension.ts Outdated Show resolved Hide resolved
extensions/github/src/extension.ts Outdated Show resolved Hide resolved
extensions/github/src/extension.ts Outdated Show resolved Hide resolved
extensions/github/src/extension.ts Outdated Show resolved Hide resolved
extensions/github/src/extension.ts Outdated Show resolved Hide resolved
axel7083 and others added 5 commits January 5, 2024 14:16
@axel7083 @dgolovin
Update extensions/github/src/extension.ts
8649dcd 
Co-authored-by: Denis Golovin <dgolovin@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 @dgolovin
Update extensions/github/src/extension.ts
185df5f 
Co-authored-by: Denis Golovin <dgolovin@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 @dgolovin
Update extensions/github/src/extension.ts
d0df638 
Co-authored-by: Denis Golovin <dgolovin@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 @dgolovin
Update extensions/github/src/extension.ts
6fdd457 
Co-authored-by: Denis Golovin <dgolovin@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 @dgolovin
Update extensions/github/src/extension.ts
65b3fc8 
Co-authored-by: Denis Golovin <dgolovin@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 axel7083 self-assigned this Jan 5, 2024
@axel7083
revert: changes to kind and compose extension
76ebb89 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083 axel7083 linked an issue Jan 5, 2024 that may be closed by this pull request
github API rate limit exceeded #4079
Open
@axel7083
fix: format
d00f815 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083
Copy link
Contributor Author

axel7083 commented Jan 5, 2024 •
edited

  1. There is not need for global var to store disposable, just use extensionContext.subscriptions
    Thanks for the tip!
  1. Refactoring for king and compose should be in additional PR.

I created a separate branch https://github.com/axel7083/podman-desktop/tree/refactor/kind-compose-github-provider I will open a PR when this one this be merged

CC @dgolovin

@axel7083 axel7083 mentioned this pull request Jan 5, 2024
Closed
@odockal odockal mentioned this pull request Jan 10, 2024
Open
5 tasks
benoitf
benoitf requested changes Jan 25, 2024
View reviewed changes
extensions/github/src/extension.ts Outdated Show resolved Hide resolved
extensions/github/tsconfig.json Outdated Show resolved Hide resolved
extensions/github/src/extension.ts Outdated Show resolved Hide resolved
extensions/github/src/extension.ts
// If we have a defined GitHub preference access token, let's create the session
if (getGitHubAccessToken()) {
try {
await authProvider.createSession([]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

it does not need any scopes ?

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 do not see we could scope it ?

package.json Show resolved Hide resolved
@axel7083
fix: resolving comments
8d50b64 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@axel7083
fix: resolving comments
2539606 
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
dgolovin
dgolovin requested changes Feb 1, 2024
View reviewed changes
extensions/github/src/extension.ts
}

export function getGitHubAccessToken(): string | undefined {
return extensionApi.configuration.getConfiguration('github').get('token');
Copy link
Contributor

Choose a reason for hiding this comment

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

A github token should be treated the same way as username/password pair. It cannot be stored in configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benoitf do you have an idea on the way to proceed because I am a bit lost

Copy link
Contributor

Choose a reason for hiding this comment

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

Extension context should have secrets property to store secrets and that should be encrypted storage.
safeStorage and safeStorageElectron in electron that can help with encryption.

Example how to use it from VS Code https://vscode.dev/github/microsoft/vscode/blob/main/src/vs/platform/encryption/electron-main/encryptionMainService.ts#L32

@odockal
Copy link
Contributor

odockal commented Feb 14, 2024

@axel7083 Hey, I have prepared an epic to cover this feature (#5488), it would be nice if (I haven't fully checked every comment) we have acceptance criteria of this feature, so we are sure to cover everything with e2e tests in future. thx!

@axel7083 axel7083 marked this pull request as draft February 26, 2024 08:52
@xbabalov xbabalov mentioned this pull request Apr 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgolovin dgolovin dgolovin requested changes

@benoitf benoitf benoitf requested changes

@lstocchi lstocchi lstocchi approved these changes

@cdrage cdrage Awaiting requested review from cdrage cdrage is a code owner automatically assigned from containers/podman-desktop-reviewers

Requested changes must be addressed to merge this pull request.

Assignees

@axel7083 axel7083

Labels
None yet
Projects
Podman Desktop Planning
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

github API rate limit exceeded
5 participants
@axel7083 @benoitf @odockal @dgolovin @lstocchi