Merge pull request #6848 from cli/jungaretti/fix-jupyter-spinner

Fix progress indicator bug with generic RunWithProgress function

pkg/cmd/codespace/common.go

@@ -59,6 +59,10 @@ func (a *App) StopProgressIndicator() {
 	a.io.StopProgressIndicator()
 }
 
+func (a *App) RunWithProgress(label string, run func() error) error {
+	return a.io.RunWithProgress(label, run)
+}
+
 // Connects to a codespace using Live Share and returns that session
 func startLiveShareSession(ctx context.Context, codespace *api.Codespace, a *App, debug bool, debugFile string) (session *liveshare.Session, err error) {
 	liveshareLogger := noopLogger()

pkg/cmd/codespace/create.go

@@ -154,17 +154,20 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 		userInputs.Location = vscsLocation
 	}
 
-	a.StartProgressIndicatorWithLabel("Fetching repository")
-	repository, err := a.apiClient.GetRepository(ctx, userInputs.Repository)
-	a.StopProgressIndicator()
+	var repository *api.Repository
+	err := a.RunWithProgress("Fetching repository", func() (err error) {
+		repository, err = a.apiClient.GetRepository(ctx, userInputs.Repository)
+		return
+	})
 	if err != nil {
 		return fmt.Errorf("error getting repository: %w", err)
 	}
 
-	a.StartProgressIndicatorWithLabel("Validating repository for codespaces")
-	billableOwner, err := a.apiClient.GetCodespaceBillableOwner(ctx, userInputs.Repository)
-	a.StopProgressIndicator()
-
+	var billableOwner *api.User
+	err = a.RunWithProgress("Validating repository for codespaces", func() (err error) {
+		billableOwner, err = a.apiClient.GetCodespaceBillableOwner(ctx, userInputs.Repository)
+		return
+	})
 	if err != nil {
 		return fmt.Errorf("error checking codespace ownership: %w", err)
 	} else if billableOwner != nil && (billableOwner.Type == "Organization" || billableOwner.Type == "User") {
@@ -201,9 +204,11 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 
 	// now that we have repo+branch, we can list available devcontainer.json files (if any)
 	if opts.devContainerPath == "" {
-		a.StartProgressIndicatorWithLabel("Fetching devcontainer.json files")
-		devcontainers, err := a.apiClient.ListDevContainers(ctx, repository.ID, branch, 100)
-		a.StopProgressIndicator()
+		var devcontainers []api.DevContainerEntry
+		err = a.RunWithProgress("Fetching devcontainer.json files", func() (err error) {
+			devcontainers, err = a.apiClient.ListDevContainers(ctx, repository.ID, branch, 100)
+			return
+		})
 		if err != nil {
 			return fmt.Errorf("error getting devcontainer.json paths: %w", err)
 		}
@@ -266,9 +271,11 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 		DisplayName:            opts.displayName,
 	}
 
-	a.StartProgressIndicatorWithLabel("Creating codespace")
-	codespace, err := a.apiClient.CreateCodespace(ctx, createParams)
-	a.StopProgressIndicator()
+	var codespace *api.Codespace
+	err = a.RunWithProgress("Creating codespace", func() (err error) {
+		codespace, err = a.apiClient.CreateCodespace(ctx, createParams)
+		return
+	})
 
 	if err != nil {
 		var aerr api.AcceptPermissionsRequiredError
@@ -355,9 +362,11 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api
 	// we can continue with the create opting out of the additional permissions
 	createParams.PermissionsOptOut = true
 
-	a.StartProgressIndicatorWithLabel("Creating codespace")
-	codespace, err := a.apiClient.CreateCodespace(ctx, createParams)
-	a.StopProgressIndicator()
+	var codespace *api.Codespace
+	err := a.RunWithProgress("Creating codespace", func() (err error) {
+		codespace, err = a.apiClient.CreateCodespace(ctx, createParams)
+		return
+	})
 
 	if err != nil {
 		return nil, fmt.Errorf("error creating codespace: %w", err)

pkg/cmd/codespace/delete.go

@@ -89,17 +89,18 @@ func (a *App) Delete(ctx context.Context, opts deleteOptions) (err error) {
 	var codespaces []*api.Codespace
 	nameFilter := opts.codespaceName
 	if nameFilter == "" {
-		a.StartProgressIndicatorWithLabel("Fetching codespaces")
-		userName := opts.userName
-		if userName == "" && opts.orgName != "" {
-			currentUser, err := a.apiClient.GetUser(ctx)
-			if err != nil {
-				return err
+		err = a.RunWithProgress("Fetching codespaces", func() (fetchErr error) {
+			userName := opts.userName
+			if userName == "" && opts.orgName != "" {
+				currentUser, fetchErr := a.apiClient.GetUser(ctx)
+				if fetchErr != nil {
+					return fetchErr
+				}
+				userName = currentUser.Login
 			}
-			userName = currentUser.Login
-		}
-		codespaces, err = a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{OrgName: opts.orgName, UserName: userName})
-		a.StopProgressIndicator()
+			codespaces, fetchErr = a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{OrgName: opts.orgName, UserName: userName})
+			return
+		})
 		if err != nil {
 			return fmt.Errorf("error getting codespaces: %w", err)
 		}
@@ -113,17 +114,15 @@ func (a *App) Delete(ctx context.Context, opts deleteOptions) (err error) {
 			nameFilter = c.Name
 		}
 	} else {
-		a.StartProgressIndicatorWithLabel("Fetching codespace")
-
 		var codespace *api.Codespace
-		var err error
-
-		if opts.orgName == "" || opts.userName == "" {
-			codespace, err = a.apiClient.GetCodespace(ctx, nameFilter, false)
-		} else {
-			codespace, err = a.apiClient.GetOrgMemberCodespace(ctx, opts.orgName, opts.userName, opts.codespaceName)
-		}
-		a.StopProgressIndicator()
+		err := a.RunWithProgress("Fetching codespace", func() (fetchErr error) {
+			if opts.orgName == "" || opts.userName == "" {
+				codespace, fetchErr = a.apiClient.GetCodespace(ctx, nameFilter, false)
+			} else {
+				codespace, fetchErr = a.apiClient.GetOrgMemberCodespace(ctx, opts.orgName, opts.userName, opts.codespaceName)
+			}
+			return
+		})
 		if err != nil {
 			return fmt.Errorf("error fetching codespace information: %w", err)
 		}
@@ -169,25 +168,25 @@ func (a *App) Delete(ctx context.Context, opts deleteOptions) (err error) {
 	if len(codespacesToDelete) > 1 {
 		progressLabel = "Deleting codespaces"
 	}
-	a.StartProgressIndicatorWithLabel(progressLabel)
-	defer a.StopProgressIndicator()
-
-	var g errgroup.Group
-	for _, c := range codespacesToDelete {
-		codespaceName := c.Name
-		g.Go(func() error {
-			if err := a.apiClient.DeleteCodespace(ctx, codespaceName, opts.orgName, opts.userName); err != nil {
-				a.errLogger.Printf("error deleting codespace %q: %v\n", codespaceName, err)
-				return err
-			}
-			return nil
-		})
-	}
 
-	if err := g.Wait(); err != nil {
-		return errors.New("some codespaces failed to delete")
-	}
-	return nil
+	return a.RunWithProgress(progressLabel, func() error {
+		var g errgroup.Group
+		for _, c := range codespacesToDelete {
+			codespaceName := c.Name
+			g.Go(func() error {
+				if err := a.apiClient.DeleteCodespace(ctx, codespaceName, opts.orgName, opts.userName); err != nil {
+					a.errLogger.Printf("error deleting codespace %q: %v\n", codespaceName, err)
+					return err
+				}
+				return nil
+			})
+		}
+
+		if err := g.Wait(); err != nil {
+			return errors.New("some codespaces failed to delete")
+		}
+		return nil
+	})
 }
 
 func confirmDeletion(p prompter, apiCodespace *api.Codespace, isInteractive bool) (bool, error) {

pkg/cmd/codespace/edit.go

@@ -54,12 +54,13 @@ func (a *App) Edit(ctx context.Context, opts editOptions) error {
 		return fmt.Errorf("error choosing codespace: %w", err)
 	}
 
-	a.StartProgressIndicatorWithLabel("Editing codespace")
-	_, err = a.apiClient.EditCodespace(ctx, codespaceName, &api.EditCodespaceParams{
-		DisplayName: opts.displayName,
-		Machine:     opts.machine,
+	err = a.RunWithProgress("Editing codespace", func() (err error) {
+		_, err = a.apiClient.EditCodespace(ctx, codespaceName, &api.EditCodespaceParams{
+			DisplayName: opts.displayName,
+			Machine:     opts.machine,
+		})
+		return
 	})
-	a.StopProgressIndicator()
 	if err != nil {
 		return fmt.Errorf("error editing codespace: %w", err)
 	}

pkg/cmd/codespace/jupyter.go

@@ -45,18 +45,24 @@ func (a *App) Jupyter(ctx context.Context, selector *CodespaceSelector) (err err
 	}
 	defer safeClose(session, &err)
 
-	a.StartProgressIndicatorWithLabel("Starting JupyterLab on codespace")
-	invoker, err := rpc.CreateInvoker(ctx, session)
+	serverPort, serverUrl := 0, ""
+	err = a.RunWithProgress("Starting JupyterLab on codespace", func() (err error) {
+		invoker, err := rpc.CreateInvoker(ctx, session)
+		if err != nil {
+			return
+		}
+
+		err = invoker.Close()
+		if err != nil {
+			return
+		}
+
+		serverPort, serverUrl, err = invoker.StartJupyterServer(ctx)
+		return
+	})
 	if err != nil {
 		return err
 	}
-	defer safeClose(invoker, &err)
-
-	serverPort, serverUrl, err := invoker.StartJupyterServer(ctx)
-	if err != nil {
-		return fmt.Errorf("failed to start JupyterLab server: %w", err)
-	}
-	a.StopProgressIndicator()
 
 	// Pass 0 to pick a random port
 	listen, _, err := codespaces.ListenTCP(0)

pkg/cmd/codespace/list.go

@@ -62,9 +62,11 @@ func (a *App) List(ctx context.Context, opts *listOptions, exporter cmdutil.Expo
 		return cmdutil.FlagErrorf("using `--org` or `--user` with `--repo` is not allowed")
 	}
 
-	a.StartProgressIndicatorWithLabel("Fetching codespaces")
-	codespaces, err := a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{Limit: opts.limit, RepoName: opts.repo, OrgName: opts.orgName, UserName: opts.userName})
-	a.StopProgressIndicator()
+	var codespaces []*api.Codespace
+	err := a.RunWithProgress("Fetching codespaces", func() (err error) {
+		codespaces, err = a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{Limit: opts.limit, RepoName: opts.repo, OrgName: opts.orgName, UserName: opts.userName})
+		return
+	})
 	if err != nil {
 		return fmt.Errorf("error getting codespaces: %w", err)
 	}

pkg/cmd/codespace/logs.go

@@ -55,15 +55,21 @@ func (a *App) Logs(ctx context.Context, selector *CodespaceSelector, follow bool
 	}
 	defer listen.Close()
 
-	a.StartProgressIndicatorWithLabel("Fetching SSH Details")
-	invoker, err := rpc.CreateInvoker(ctx, session)
-	if err != nil {
-		return err
-	}
-	defer safeClose(invoker, &err)
+	remoteSSHServerPort, sshUser := 0, ""
+	err = a.RunWithProgress("Fetching SSH Details", func() (err error) {
+		invoker, err := rpc.CreateInvoker(ctx, session)
+		if err != nil {
+			return
+		}
+
+		err = invoker.Close()
+		if err != nil {
+			return
+		}
 
-	remoteSSHServerPort, sshUser, err := invoker.StartSSHServer(ctx)
-	a.StopProgressIndicator()
+		remoteSSHServerPort, sshUser, err = invoker.StartSSHServer(ctx)
+		return
+	})
 	if err != nil {
 		return fmt.Errorf("error getting ssh server details: %w", err)
 	}

pkg/cmd/codespace/ports.go

@@ -68,9 +68,11 @@ func (a *App) ListPorts(ctx context.Context, selector *CodespaceSelector, export
 	}
 	defer safeClose(session, &err)
 
-	a.StartProgressIndicatorWithLabel("Fetching ports")
-	ports, err := session.GetSharedServers(ctx)
-	a.StopProgressIndicator()
+	var ports []*liveshare.Port
+	err = a.RunWithProgress("Fetching ports", func() (err error) {
+		ports, err = session.GetSharedServers(ctx)
+		return
+	})
 	if err != nil {
 		return fmt.Errorf("error getting ports of shared servers: %w", err)
 	}
@@ -276,40 +278,40 @@ func (a *App) UpdatePortVisibility(ctx context.Context, selector *CodespaceSelec
 
 	// TODO: check if port visibility can be updated in parallel instead of sequentially
 	for _, port := range ports {
-		a.StartProgressIndicatorWithLabel(fmt.Sprintf("Updating port %d visibility to: %s", port.number, port.visibility))
+		err := a.RunWithProgress(fmt.Sprintf("Updating port %d visibility to: %s", port.number, port.visibility), func() (err error) {
+			// wait for success or failure
+			g, ctx := errgroup.WithContext(ctx)
+			ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+			defer cancel()
 
-		// wait for success or failure
-		g, ctx := errgroup.WithContext(ctx)
-		ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
-		defer cancel()
+			g.Go(func() error {
+				updateNotif, err := session.WaitForPortNotification(ctx, port.number, liveshare.PortChangeKindUpdate)
+				if err != nil {
+					return fmt.Errorf("error waiting for port %d to update: %w", port.number, err)
 
-		g.Go(func() error {
-			updateNotif, err := session.WaitForPortNotification(ctx, port.number, liveshare.PortChangeKindUpdate)
-			if err != nil {
-				return fmt.Errorf("error waiting for port %d to update: %w", port.number, err)
+				}
+				if !updateNotif.Success {
+					if updateNotif.StatusCode == http.StatusForbidden {
+						return newErrUpdatingPortVisibility(port.number, port.visibility, errUpdatePortVisibilityForbidden)
+					}
+					return newErrUpdatingPortVisibility(port.number, port.visibility, errors.New(updateNotif.ErrorDetail))
 
-			}
-			if !updateNotif.Success {
-				if updateNotif.StatusCode == http.StatusForbidden {
-					return newErrUpdatingPortVisibility(port.number, port.visibility, errUpdatePortVisibilityForbidden)
 				}
-				return newErrUpdatingPortVisibility(port.number, port.visibility, errors.New(updateNotif.ErrorDetail))
+				return nil // success
+			})
 
-			}
-			return nil // success
-		})
+			g.Go(func() error {
+				err := session.UpdateSharedServerPrivacy(ctx, port.number, port.visibility)
+				if err != nil {
+					return fmt.Errorf("error updating port %d to %s: %w", port.number, port.visibility, err)
+				}
+				return nil
+			})
 
-		g.Go(func() error {
-			err := session.UpdateSharedServerPrivacy(ctx, port.number, port.visibility)
-			if err != nil {
-				return fmt.Errorf("error updating port %d to %s: %w", port.number, port.visibility, err)
-			}
-			return nil
+			// wait for success or failure
+			err = g.Wait()
+			return
 		})
-
-		// wait for success or failure
-		err := g.Wait()
-		a.StopProgressIndicator()
 		if err != nil {
 			return err
 		}