Skip to content

App Hosting: GitHub integration improvements#7405

Merged
mathu97 merged 12 commits intomasterfrom
refactor/github-connections-poc
Jul 16, 2024
Merged

App Hosting: GitHub integration improvements#7405
mathu97 merged 12 commits intomasterfrom
refactor/github-connections-poc

Conversation

@mathu97
Copy link
Copy Markdown
Contributor

@mathu97 mathu97 commented Jul 1, 2024

  1. UX Update: Prompt for GitHub account / org first, then list repos for that account / org
  2. Go directly to GitHub to install Firebase App Hosting GitHub app on a new account
  3. Go directly to GitHub to manage an existing installation
@mathu97 mathu97 changed the title Refactor/GitHub connections poc Jul 10, 2024
@mathu97 mathu97 marked this pull request as ready for review July 10, 2024 03:40
@mathu97 mathu97 force-pushed the refactor/github-connections-poc branch from 941230a to d7f2de2 Compare July 10, 2024 14:08
@mathu97 mathu97 requested review from qtz1, taeold and tonyjhuang July 10, 2024 14:28
await createFullyInstalledConnection(projectId, location, generateConnectionId(), oauthConn),
while (installationId === ADD_ACCOUNT_CHOICE) {
utils.logBullet(
"Install the Firebase App Hosting GitHub app on a new account to enable access to those repositories",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit* not blocking

I wonder how surprising it is for users to ultimately be redirected to DevConnect. It looks friendly enough, and even says you should close the page, but I wonder if we should give a hint here about what to expect when a popup is open.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmm we could add some messaging here but I feel that this would make it too verbose. Ideally we redirect to a firebase branded success page which we can look into separately.

return conn;
}

async function manageInstallation(connection: devConnect.Connection): Promise<void> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit* it looks like "manage" is an intentional choice of word here but I was confused because I expected it to be "update" or "configure". Curious why you ended up deciding on "manage".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought "manage" makes sense because it takes them to the installation page from which users can either, update the installation's repository access, suspend the installation, or uninstall the GitHub app. I think "configure" could work here as well.

Screenshot 2024-07-11 at 9 47 43 AM
Copy link
Copy Markdown
Contributor

@taeold taeold left a comment

Choose a reason for hiding this comment

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

Can you add a changelog entry?

@mathu97 mathu97 enabled auto-merge (squash) July 16, 2024 20:15
@mathu97 mathu97 merged commit 8ed240a into master Jul 16, 2024
@mathu97 mathu97 deleted the refactor/github-connections-poc branch July 17, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants