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

App Hosting: GitHub integration improvements #7405

Merged
merged 12 commits into from
Jul 16, 2024

Conversation

mathu97
Copy link
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 App Hosting: GitHub integration improvements 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
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
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
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.

@@ -180,6 +195,107 @@ async function createFullyInstalledConnection(
return conn;
}

async function manageInstallation(connection: devConnect.Connection): Promise<void> {
Copy link
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
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

src/apphosting/githubConnections.ts Outdated Show resolved Hide resolved
src/apphosting/githubConnections.ts Outdated Show resolved Hide resolved
src/apphosting/githubConnections.ts Outdated Show resolved Hide resolved
Copy link
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
41 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants