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

Improve getting started experience #7011

Merged
merged 5 commits into from May 6, 2024
Merged

Improve getting started experience #7011

merged 5 commits into from May 6, 2024

Conversation

inlined
Copy link
Member

@inlined inlined commented Apr 16, 2024

  1. Tells customers they can ^C once the rollout has been created
  2. Turns the rollout in progress message to a spinny
  3. Adds a spinny to wait on TLS to be ready. To be forwards compatible with the day that we can just serve all default domains on a wildcard cert, this dialog only pops up after an initial check verifies that TLS is not yet ready.

Not sure about some of the prose I've moved about. Feel encouraged to nitpick and/or add other nitpickers as reviewers.

@inlined
Copy link
Member Author

inlined commented May 3, 2024

Reduced scope to just awaiting TLS now that other changes have been implemented. Changed the rollout text a bit since we may to continue waiting for TLS. Also, I removed a "..." because it seems redundant/passive aggressive along with a spinner.

Copy link
Contributor

@tonyjhuang tonyjhuang left a comment

Choose a reason for hiding this comment

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

Left some nits

@@ -121,6 +148,7 @@ export async function doSetup(
});

await setDefaultTrafficPolicy(projectId, location, backendId, branch);
logSuccess(`Successfully created backend:\n\t${backend.name}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, this is already on L138

Copy link
Member Author

Choose a reason for hiding this comment

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

ugh. merge issues. I had previously moved and now it got turned into an add. TY for the catch.

createRolloutSpinner.succeed(`Your backend is now deployed at:\n\thttps://${backend.uri}`);
createRolloutSpinner.succeed("Rollout complete");
if (!(await tlsReady(url))) {
const tlsSpinner = ora("Waiting for TLS certificate");
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably want tlsSpinner = ora(...).start()

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! Maybe Ora isn't as easy to use as I thought!

const createRolloutSpinner = ora(
"Starting a new rollout... This make take a few minutes. It's safe to exit now.",
"Starting a new rollout; this make take a few minutes. It's safe to exit now.",
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: this may take

createRolloutSpinner.succeed(`Your backend is now deployed at:\n\thttps://${backend.uri}`);
createRolloutSpinner.succeed("Rollout complete");
if (!(await tlsReady(url))) {
const tlsSpinner = ora("Waiting for TLS certificate");
Copy link
Contributor

Choose a reason for hiding this comment

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

"Finalizing your backend's TLS certificate; this may take a few minutes."

@inlined inlined enabled auto-merge (squash) May 6, 2024 17:42
@inlined inlined merged commit 4822aee into master May 6, 2024
35 checks passed
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

2 participants