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

fix: new project widget broken if 'null' item(s) received from gh api #11630

Merged
merged 1 commit into from
Jul 26, 2022
Merged

fix: new project widget broken if 'null' item(s) received from gh api #11630

merged 1 commit into from
Jul 26, 2022

Conversation

szab100
Copy link
Contributor

@szab100 szab100 commented Jul 25, 2022

Description

The |Team or persona scope| / Projects / New Project widget has been super slow since the feature's debut when running against large Github Enterprise instances with 10s of thousands of repositories under many-100 git-orgs. Since the current implementation is filtering to show only the repos where the current user has 'admin' permissions only after all pages are fetched and it makes the UI wait for the entire result set to be fetched before returning, the UI is frozen for 5-10+ minutes.

Still, even as being frozen / super slow (added a TODO to fix / optimize), this feature remained usable, since it is a one-time project setup. However, since we upgraded to GH Enterprise v 3.x, it seems that some returned pages might contain one or more null items, where the current filter expression fails. This PR simply aims at fixing this blocking issue, while not giving a remedy yet for the slowness of this API call.

Related Issue(s)

#11628
#11629

Fixes #
#11629

How to test

The fix should be trivial, simply ensure the feature still works after applying this change.

Release Notes

fix: new project widget broken if 'null' item(s) received from gh api

Copy link
Contributor

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

Thanks @szab100. The change looks good. This feels like a bug in GHE or octokit - perhaps worth following up with those projects.

/hold for CLA

@meysholdt
Copy link
Member

hi @szab100, and thank you for your contribution! Can you please sign our CLA via this DocuSign form? If there are any questions, you can reach me via moritz@gitpod.io.

@szab100
Copy link
Contributor Author

szab100 commented Jul 26, 2022

hi @szab100, and thank you for your contribution! Can you please sign our CLA via this DocuSign form? If there are any questions, you can reach me via moritz@gitpod.io.

done.

@meysholdt meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Jul 26, 2022
@meysholdt
Copy link
Member

Thx for signing the CLA!

@szab100
Copy link
Contributor Author

szab100 commented Jul 26, 2022

Thanks @szab100. The change looks good. This feels like a bug in GHE or octokit - perhaps worth following up with those projects.

/hold for CLA

I agree, that octokit client lib should never really return 'null' values, perhaps it is due to failed / retried paginated requests.

About the other mentioned issue (performance): it would be nice to see an improvement on that too soon, a few more comments / suggestions for that:

  • a pure typeahead search would probably provide a great user experience as it could completely eliminate waiting..
  • we might want to return the current_user != admin repos as well, only grey them out on the UI (with add'l server-side check when submitted), with a comment explaining this action requires admin => less confusing than not seeing the repository the user is looking for.
  • returning a stream could be good too, the UI could interactively load the results & show the progress (not sure if octokit returns the counter or # of total pages upfront to show a deterministic progress bar).
  • I have not tested, but I believe even just adding the 'affiliation: collaborator' (server-side) filter to the current octokit call would make it much better than today as - although it is a lower permission level than we need - it should greatly decrease the number of pages to fetch & further filter on, skipping the prob 10s of thousands of repositories where the user is not even a collaborator (but I am not sure if having 'admin' permission always includes being a 'collaborator' as well).

@laushinka
Copy link
Contributor

laushinka commented Jul 26, 2022

/werft run

👍 started the job as gitpod-build-fix-new-project-widget-fork.0
(with .werft/ from main)

@roboquat roboquat merged commit 98acacb into gitpod-io:main Jul 26, 2022
@szab100 szab100 deleted the fix_new_project_widget branch July 26, 2022 23:24
@andrew-farries
Copy link
Contributor

andrew-farries commented Jul 27, 2022

a few more comments / suggestions for that:

Thanks for these 👍. I've added them to #11628 so they don't get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants