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(webserver, db): Implement github provided repositories #1800

Merged
merged 15 commits into from
Apr 16, 2024
Merged

feat(webserver, db): Implement github provided repositories #1800

merged 15 commits into from
Apr 16, 2024

Conversation

boxbeam
Copy link
Contributor

@boxbeam boxbeam commented Apr 9, 2024 •
edited
Loading

cc @liangfung

Relates to TAB-479

@boxbeam boxbeam requested a review from wsxiaoys April 9, 2024 23:26
@codecov Codecov
Copy link

codecov bot commented Apr 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 72.64957% with 64 lines in your changes are missing coverage. Please review.

Project coverage is 55.08%. Comparing base (f21b900) to head (b6fcc56).
Report is 12 commits behind head on main.

❗ Current head b6fcc56 differs from pull request most recent head 271d4d8. Consider uploading reports for the commit 271d4d8 to get more accurate results

Files Patch % Lines
ee/tabby-db/src/github_repository_provider.rs 76.71% 17 Missing ⚠️
ee/tabby-webserver/src/schema/mod.rs 0.00% 16 Missing ⚠️
...webserver/src/schema/github_repository_provider.rs 0.00% 15 Missing ⚠️
...ebserver/src/service/github_repository_provider.rs 94.64% 6 Missing ⚠️
ee/tabby-webserver/src/handler.rs 0.00% 5 Missing ⚠️
ee/tabby-webserver/src/service/dao.rs 72.72% 3 Missing ⚠️
ee/tabby-webserver/src/integrations/github.rs 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
+ Coverage   54.48%   55.08%   +0.59%     
==========================================
  Files         121      123       +2     
  Lines       10753    10999     +246     
==========================================
+ Hits         5859     6059     +200     
- Misses       4894     4940      +46

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wsxiaoys
wsxiaoys reviewed Apr 10, 2024
View reviewed changes
Copy link
Member

@wsxiaoys wsxiaoys left a comment

Choose a reason for hiding this comment

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

Please also integrate RepositoryAccess in this PR for github provided repositories.

ee/tabby-db/migrations/0024_github-provided-repos.up.sql Outdated Show resolved Hide resolved
ee/tabby-db/migrations/0024_github-provided-repos.up.sql Outdated Show resolved Hide resolved
ee/tabby-db/migrations/0024_github-provided-repos.up.sql Outdated Show resolved Hide resolved
ee/tabby-webserver/src/schema/mod.rs Outdated Show resolved Hide resolved
@wsxiaoys wsxiaoys marked this pull request as draft April 10, 2024 01:13
@boxbeam boxbeam force-pushed the github-provider-repositories branch 2 times, most recently from fff1fd4 to 9a9b631 Compare April 10, 2024 02:52
@boxbeam boxbeam marked this pull request as ready for review April 10, 2024 02:54
wsxiaoys
wsxiaoys requested changes Apr 10, 2024
View reviewed changes
ee/tabby-db/migrations/0024_github-provided-repos.up.sql Show resolved Hide resolved
ee/tabby-webserver/src/hub/mod.rs Outdated Show resolved Hide resolved
ee/tabby-webserver/src/hub/mod.rs Outdated Show resolved Hide resolved
ee/tabby-webserver/src/hub/mod.rs Outdated Show resolved Hide resolved
ee/tabby-webserver/src/hub/mod.rs Outdated Show resolved Hide resolved
ee/tabby-webserver/src/hub/mod.rs Outdated Show resolved Hide resolved
@wsxiaoys wsxiaoys marked this pull request as draft April 10, 2024 14:27
@boxbeam boxbeam force-pushed the github-provider-repositories branch from 6e33d34 to bd71d56 Compare April 15, 2024 18:35
@boxbeam boxbeam marked this pull request as ready for review April 15, 2024 18:36
wsxiaoys
wsxiaoys reviewed Apr 15, 2024
View reviewed changes
Copy link
Member

@wsxiaoys wsxiaoys left a comment

Choose a reason for hiding this comment

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

Overall LG, please add unit test for coverage

@wsxiaoys wsxiaoys marked this pull request as draft April 15, 2024 20:29
@boxbeam boxbeam marked this pull request as ready for review April 16, 2024 18:05
@wsxiaoys wsxiaoys marked this pull request as draft April 16, 2024 18:15
@boxbeam boxbeam marked this pull request as ready for review April 16, 2024 18:42
@wsxiaoys wsxiaoys enabled auto-merge (squash) April 16, 2024 18:43
wsxiaoys
wsxiaoys reviewed Apr 16, 2024
View reviewed changes
ee/tabby-webserver/src/schema/github_repository_provider.rs
@@ -11,6 +11,8 @@ pub struct GithubRepositoryProvider {
pub id: ID,
pub display_name: String,
pub application_id: String,
#[graphql(skip)]
pub access_token: Option<String>,
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
pub access_token: Option<String>,
pub access_token: String,

Copy link
Contributor Author

@boxbeam boxbeam Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The access token can be None (if the user has not gone through the OAuth connect flow yet), and this will only be used internally. I think we should prefer a closer representation of the data, since otherwise I'll be having to check if access_token.is_empty() instead.

@wsxiaoys wsxiaoys merged commit fc2efd7 into main Apr 16, 2024
5 of 6 checks passed
@wsxiaoys wsxiaoys deleted the github-provider-repositories branch April 16, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wsxiaoys wsxiaoys wsxiaoys approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@boxbeam @wsxiaoys