feat: Add and edit github connection #5263

AdityaHegde · 2024-07-12T13:31:18Z

Notion: https://www.notion.so/rilldata/RD-RC-1-3-Project-Status-Page-updates-dd5daf216f5a4c05886b98bc20d4f8f5

ericpgreen2 · 2024-07-18T02:43:16Z

Some initial UXQA –

Editing the GitHub repo is buggy. The GitHub repo rendered in ProjectGitHubConnection.svelte never updates. And the listed resources appear to be linked to the previous (t-1) GitHub repo, not the current (t) GitHub repo. I'll send a Jam via Slack.
The dropdown should be initialized with the current GitHub repository (in this case, rill-examples)
@ericokuma, it looks like we need to add a field to configure the subpath. FWIW, here's how Vercel handles selecting a "root directory":

web-admin/src/routes/-/github/connect/+page.svelte

web-admin/src/features/projects/github/GithubRepoInline.svelte

ericpgreen2 · 2024-07-18T02:14:11Z

web-admin/src/features/projects/github/GithubRepoUpdater.ts

+      if (userStatus.isFetching || userRepos.isFetching) {
+        return {
+          isLoading: true,
+          error: undefined,
+        };
+      }


We should be consistent with terminology. If reading isFetching, then we should return isFetching. If reading isLoading, then we should return isLoading.

ericpgreen2 · 2024-07-18T02:27:14Z

web-admin/src/features/projects/github/GithubRepoSelectionDialog.svelte

+  } from "@rilldata/web-common/components/alert-dialog";
+  import { Button } from "@rilldata/web-common/components/button";
+  import Github from "@rilldata/web-common/components/icons/Github.svelte";
+  import Select from "@rilldata/web-common/components/forms/Select.svelte";


A little out-of-scope of this PR, but it'd be nice to deprecate this in favor of ShadCN's Select component

ericpgreen2 · 2024-07-18T02:36:41Z

web-admin/src/features/projects/github/GithubConnection.ts

IMO this extra layer of abstraction isn't necessary & adds complexity.

Is there something wrong with using vanilla TanStack Query in the ProjectGitHubConnection.svelte component? That's the pattern we're using most frequently throughout the codebase.

ericpgreen2 · 2024-07-18T02:42:23Z

web-admin/src/features/projects/github/GithubRepoUpdater.ts

Similarly, is there a reason to be adding this abstraction on top of vanilla TanStack Query? To update the GitHub repo seems like a vanilla mutation and shouldn't require too much complexity. I wonder if the bugginess I shared via Jam is a derivative of this complexity.

If we're to go down this route, I'd expect a "GithubRepoUpdater" to include a method to actually update the Github repo, but that looks to be happening outside of this abstraction in the GithubRepoSelectionDialog.svelte component.

This endpoint can be slow if there are a lot of repos and orgs connected. I want to avoid refetching it on window focus all the time, but only when connect to github or choose other repos is clicked.

Both this and GithubConnection have a check to only refetch if it was explicitly asked for (basically the connecting check).

Naming this GithubRepoUpdater is just a remnant of some old code.

Gotcha. The vanilla TanStack approach would be to use $githubQuery.refetch() in the component itself. Might be simpler?

We are refetching the query right? The idea is to hide as much code outside of components as possible. We still need the check if we popped a page to select github repos or not.

This will also hopefully allow us to abstract such code out. But it will have to be follow ups

ericpgreen2 · 2024-07-19T02:20:20Z

web-admin/src/features/projects/github/GithubRepoSelectionDialog.svelte

+
+  function handleVisibilityChange() {
+    if (document.visibilityState !== "visible") return;
+    void githubReposConnection.focused();


I had to read this function implementation to know what it was doing. If we put the Query Observer in this component, we could use $githubUserReposQuery.refetch(), which would be more explicit.

ericpgreen2 · 2024-07-19T02:27:40Z

web-admin/src/features/projects/github/GithubRepoUpdater.ts

Gotcha. The vanilla TanStack approach would be to use $githubQuery.refetch() in the component itself. Might be simpler?

ericpgreen2

Some more UXQA:

All user-facing text that says "github" or "Github" should be "GitHub"
After triggering the connect-to-GitHub flow via the UI, it looks like this page should say "... continue setup in your original tab."
I get back 150 repos from the ListGithubUserRepos API, but the Select component only shows me the first 100.

ericpgreen2 · 2024-07-19T02:33:50Z

web-admin/src/features/projects/github/ProjectGithubConnection.svelte

+
+  function confirmConnectToGithub() {
+    confirmDialogOpen = false;
+    void githubConnection.check();


Here as well, I had to read the function implementation to understand what was going on. What about just

const githubUserStatusQuery = createAdminServiceGetGithubUserStatus(); ... window.open( $githubUserStatusQuery.data?.grantAccessUrl, "_blank", )

ericpgreen2 · 2024-07-19T17:55:16Z

Writing down what we discussed live:

The motivation behind the GitHubConnection classes is to track the state of the connect-to-GitHub flow (which happens in a new tab). We want to be smart about when we refetch GetGitHubUserStatus and ListGitHubUserRepos, each of which can take a couple seconds. The refetches should happen after the connect-to-GitHub flow on page re-focus. However, these GitHubConnection classes are hard to read.
We should know why updating the GitHub repo leads to a page flash. We figured out it's the result of a GetProject request that omits cookies & returns a 401 error. But why does the request not include cookies?

ericpgreen2 · 2024-07-24T20:12:43Z

FYI, @jkhwu updated the designs to remove a couple cases of indentation. E.g., see these new mocks:

The GitHub block no longer is indented by the GitHub logo. Instead the logo is inside the button:

The "select your GitHub repository" form body is no longer indented by the GitHub logo: https://www.figma.com/design/Qt6EyotCBS3V6O31jVhMQ7/RILL?node-id=14933-609154&t=PgNKhdmzo8xhGY3P-1

ericpgreen2

UXQA:

The "Are you sure you want to connect?" dialog flashes & proceeds without waiting for confirmation from the user. See Jam
After going through the "choose other repos" flow, I get brought to this page that references the CLI.

Possibly related, I've been wondering – when the connect-to-GitHub flow is complete, can we somehow communicate this to the /status page, so it knows exactly when to refetch the list of repositories? Polling doesn't feel exactly right, (but the way you've contained the polling looks good).

ericpgreen2 · 2024-07-24T20:18:01Z

web-admin/src/features/projects/github/ConnectToGithubButton.svelte

+
+  function openGithubRepoCreator() {
+    // we need a popup window so we cannot use href
+    window.open("https://github.com/new", "", "width=1024,height=600");


Nit: we can name the window (2nd parameter) githubWindow. I had to Google what the empty string represented.

ericpgreen2 · 2024-07-24T20:55:00Z

web-admin/src/features/projects/github/GithubData.ts

+    this.openGithubConnectWindow(userStatus.grantAccessUrl);
+  }
+
+  public async refetch() {


I think this function is only used internally – so it could be private?

ericpgreen2 · 2024-07-29T13:35:46Z

UXQA –

After the "Choose another repo" flow, I see this page that references the CLI (cc @ericokuma):
I've followed the "subpath" placeholder text that advised to add a forward slash. It results in a double forward slash in the display.
I tried to edit the GitHub connection, I created a new blank repo, selected it in the dropdown, and clicked "Continue". See the 400 error below. If this flow is meant to write the files to the new repo, then it failed. If this flow is just meant to read the new repo's contents, then maybe we should have a custom error message that says something like "Repository is empty. Please select a repository that contains your Rill code files."
I've added a file to my repo rill-test-ui-connect-to-github-2 so that it's non-empty. I confirmed my intention to overwrite and clicked "Continue." I hit the below 400 error.

k-anshul · 2024-07-30T15:05:52Z

admin/server/github.go

+
+	claims := auth.GetClaims(ctx)
+	if !claims.ProjectPermissions(ctx, proj.OrganizationID, proj.ID).ManageProject {
+		return nil, status.Error(codes.PermissionDenied, "does not have permission to delete project")


Nit: Incorrect error msg delete project

k-anshul · 2024-07-30T15:14:08Z

admin/pkg/archive/archive.go

@@ -0,0 +1,126 @@
+package archive


Nit: Should this be part of runtime/pkg since this is also used in runtime ?

k-anshul · 2024-07-30T15:17:53Z

admin/server/github.go

+		SingleBranch:  true,
+	})
+	if err != nil {
+		if errors.Is(err, transport.ErrEmptyRemoteRepository) {


Nit : Can simplify the if-else block by switching the if condition

admin/server/github.go

k-anshul · 2024-07-30T15:27:22Z

admin/server/github.go

+		return fmt.Errorf("failed to copy data: %w", err)
+	}
+
+	// add back the older gitignore contents if present


If we are anyways overwriting all other files then I think it should be okay to also overwrite .gitignore file ?

This was a requirement to not completely replace the existing file. But rather append like we do with rill init

IIRC in rill init we did not delete existing files so it made sense to not remove existing .gitignore.
Here we are deleting all existing files as well.

@nishantmonu51 @ericokuma Thoughts on the above?

k-anshul

Looks good from backend POV apart from few nits.

k-anshul · 2024-07-31T08:26:42Z

admin/server/github.go

+	defer os.RemoveAll(dir)
+
+	// use a subfolder for working with git
+	gitPath := filepath.Join(dir, "proj")


The subfolder should not be required now. We can directly use the temp_dir.

k-anshul · 2024-07-31T08:30:03Z

admin/server/github.go

+	defer cancel()
+
+	// generate a temp dir to extract the archive
+	dir, err := os.MkdirTemp(os.TempDir(), "dest_git_repos")


Given this is used for both upload and git flow so we can just change the name to project or similar?

k-anshul · 2024-07-31T08:31:56Z

admin/server/github.go

+		return err
+	}
+
+	// projPath is the target for extracting the archive


misplaced comment ?

k-anshul

Approving from backend POV

ericpgreen2 · 2024-07-31T13:38:26Z

web-admin/src/features/projects/github/GithubConnectionUpdater.ts

+  public constructor() {}
+
+  public init(githubUrl: string, subpath: string, branch: string) {
+    this.githubUrl.set(githubUrl);
+    this.subpath.set(subpath);
+    this.branch.set(branch);
+    this.isConnected = !!githubUrl;
+  }


It'd be more idiomatic to implement the constructor, rather than create a separate init function

Ya that was ideal earlier. But it is simpler to do this than create a new instance everytime.

ericpgreen2 · 2024-07-31T13:49:41Z

web-admin/src/features/projects/github/GithubData.ts

I feel like the separation of concerns between the GitHubData and GitHubConnectionUpdater classes is blurry. It might be a matter of naming & comments, or maybe all the "update" methods (startRepoSelection, reselectRepos, openGitHubConnectWindow) should live on the "Updater" class?

GitHubData is meant to be a wrapper around the user status and repos APIs. GitHubConnectionUpdater is only around the mutation to update. Both basically move code out of components as much as possible.

ericpgreen2 · 2024-07-31T13:53:00Z

web-admin/src/features/projects/github/GithubRepoSelectionDialog.svelte

+      !(await githubConnectionUpdater.update({
+        organization,
+        project,
+        force,
+        instanceId: $projectQuery.data?.prodDeployment?.runtimeInstanceId ?? "",
+      }))
+    ) {


Nit: Embedding the mutation into the conditional check makes this hard to read. I'd recommend declaring a self-describing boolean, like didUpdate.

ericpgreen2 · 2024-07-31T13:55:11Z

web-admin/src/features/projects/github/ProjectGithubConnection.svelte

+  const githubData = new GithubData();
+  setGithubData(githubData);


Should setGitHubData happen in the constructor?

setGitHubData is the component tree logic. Makes sense for it to sit here.

ericpgreen2

Approving from the frontend POV 👍

AdityaHegde added 5 commits July 12, 2024 13:17

Adding backend API to fetch connected repos

7c7c0f5

Merge branch 'main' into adityahegde/connect-to-github

f157c1e

Cleanup

3caebf5

Basics of github connection UI

708237b

Add connecting to github UI

7028a51

Base automatically changed from adityahegde/connect-to-github to main July 16, 2024 09:10

AdityaHegde added 5 commits July 16, 2024 14:42

Merge branch 'main' into adityahegde/connect-to-github-ui

c0a5baf

Add github url edit button

8f8879e

Avoid unnessary calls

64e2d30

Merge branch 'main' into adityahegde/connect-to-github-ui

c57aa21

Adjust github url edit button

ec24980

AdityaHegde requested a review from ericpgreen2 July 17, 2024 13:43

AdityaHegde marked this pull request as ready for review July 17, 2024 13:43

ericpgreen2 requested changes Jul 18, 2024

View reviewed changes

AdityaHegde force-pushed the adityahegde/connect-to-github-ui branch from 9d005a5 to 860211b Compare July 18, 2024 05:58

PR comments

546266e

AdityaHegde force-pushed the adityahegde/connect-to-github-ui branch from 860211b to 546266e Compare July 18, 2024 15:01

ericpgreen2 reviewed Jul 19, 2024

View reviewed changes

ericpgreen2 requested changes Jul 19, 2024

View reviewed changes

ericpgreen2 reviewed Jul 19, 2024

View reviewed changes

Fix invalidations

53c1b1a

ericpgreen2 requested a review from ericokuma July 19, 2024 18:07

AdityaHegde added 4 commits July 22, 2024 12:00

Fix moving to github connection

fc2bce3

Merge branch 'main' into adityahegde/connect-to-github-ui

5c3358f

Regenerate from proto

08b0376

Refactor to use a single class

21afe75

AdityaHegde force-pushed the adityahegde/connect-to-github-ui branch from d18c8e8 to 21afe75 Compare July 22, 2024 14:18

Merge branch 'main' into adityahegde/connect-to-github-ui

9012e9f

ericpgreen2 reviewed Jul 24, 2024

View reviewed changes

AdityaHegde added 2 commits July 25, 2024 15:21

Push to github for the 1st time

b5d6cdc

Happy path for push to github

61504ea

AdityaHegde force-pushed the adityahegde/connect-to-github-ui branch from d9c6d47 to 61504ea Compare July 26, 2024 06:54

Handle existing content

af24d8b

AdityaHegde force-pushed the adityahegde/connect-to-github-ui branch from 88539ad to af24d8b Compare July 26, 2024 12:34

AdityaHegde added 5 commits July 26, 2024 21:12

Fix lint and always push

69e8508

Update based on mocks

88d387e

Update connection to handle updating to new repo

83d60f8

Merge branch 'main' into adityahegde/connect-to-github-ui

079b2b1

Cleanup

ce868ee

AdityaHegde added 4 commits July 29, 2024 20:33

Fix copy on edit

f09758d

Ignore .git folder

be44162

Add telemetry

6f8caba

Fix issues around overwrite

0c17b50

k-anshul requested changes Jul 30, 2024

View reviewed changes

AdityaHegde added 3 commits July 30, 2024 21:15

Merge branch 'main' into adityahegde/connect-to-github-ui

f31a127

PR comments

880f729

Add autoclose on github connection confirmation

ce5ea9b

AdityaHegde force-pushed the adityahegde/connect-to-github-ui branch from a27c050 to ce5ea9b Compare July 31, 2024 05:30

k-anshul requested changes Jul 31, 2024

View reviewed changes

PR comments

bfeca3b

k-anshul approved these changes Jul 31, 2024

View reviewed changes

ericpgreen2 reviewed Jul 31, 2024

View reviewed changes

UI PR comments

f6c95a7

ericpgreen2 approved these changes Jul 31, 2024

View reviewed changes

AdityaHegde merged commit 8106e1a into main Jul 31, 2024
7 checks passed

AdityaHegde deleted the adityahegde/connect-to-github-ui branch July 31, 2024 14:53

		const githubData = new GithubData();
		setGithubData(githubData);

feat: Add and edit github connection #5263

feat: Add and edit github connection #5263

Conversation

AdityaHegde commented Jul 12, 2024 • edited Loading

ericpgreen2 commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaHegde Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpgreen2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpgreen2 commented Jul 19, 2024

ericpgreen2 commented Jul 24, 2024 • edited Loading

ericpgreen2 left a comment • edited Loading

Choose a reason for hiding this comment

ericpgreen2 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpgreen2 commented Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-anshul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-anshul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpgreen2 left a comment

Choose a reason for hiding this comment

AdityaHegde commented Jul 12, 2024 •

edited

Loading

AdityaHegde Jul 18, 2024 •

edited

Loading

ericpgreen2 commented Jul 24, 2024 •

edited

Loading

ericpgreen2 left a comment •

edited

Loading

ericpgreen2 Jul 24, 2024 •

edited

Loading

ericpgreen2 commented Jul 29, 2024 •

edited

Loading