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

garden up command + clean up syncs on exit #4196

Merged
merged 8 commits into from
May 12, 2023
Merged

garden up command + clean up syncs on exit #4196

merged 8 commits into from
May 12, 2023

Conversation

edvald
Copy link
Collaborator

@edvald edvald commented May 8, 2023

Co-authored by @edvald and @thsig.

See the commit messages for more details.

  • Added a --cmd option to the dev command that executes the requested command (with the provided args & opts) at the start of the interactive session.
    • Newlines in the --cmd string result in multiple commands being run in the dev command.
  • Make the garden deploy command run inside the dev command whenever it would be put into persistent operation.
  • Added a garden up command that's an alias for garden deploy --logs.
  • Any syncs started during an interactive session are now stopped when the command is exited gracefully (via exit or CTRL-D).
  • Warn the user when exiting the dev command with CTRL-C.

edvald and others added 6 commits May 11, 2023 10:23
When the `deploy` command is called with options that would put it into
persistent operation, we now run it from inside the `dev` command.

This brings a more full-featured interactive experience, and makes the
UX more consistent.
This fixes an issue where some sync-related log lines would be
duplicated (since they were rendered when the task for the relevant
deploy action emitted a `ready` event).
We now stop any syncs started during a `dev` command session when
exiting the command with CTRL-D or the `exit` command.
@thsig thsig marked this pull request as ready for review May 11, 2023 08:25
@thsig thsig changed the title garden up command garden up command + clean up syncs on exit May 11, 2023
@thsig thsig marked this pull request as draft May 11, 2023 08:30
@thsig thsig marked this pull request as ready for review May 11, 2023 09:00
@@ -156,7 +156,7 @@ Let's get your development environment wired up.
}))

function quit() {
cl?.disable("🌷 Thanks for stopping by, love you! ❤️")
cl?.disable("🌷 Thanks for stopping by! ❤️")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

💔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know : (

Copy link
Collaborator

Choose a reason for hiding this comment

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

💔

Copy link
Contributor

Choose a reason for hiding this comment

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

wait why? 💔

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was suggested that some users might find the old text too sugary.

I think it's fine myself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dropped the commit with this change, we can decide later.

Comment on lines 158 to 167
function quitWithWarning() {
log.warn(chalk.yellow(`Syncs started during this session may still be active when this command terminates. You can run ${chalk.white("garden sync stop '*'")} to stop all code syncs.`))
log.warn(chalk.yellow(`Hint: To stop code syncing on exit and allow Garden to exit gracefully, use ${chalk.white("Control-D")} or the ${chalk.white(`exit`)} command to exit ${chalk.white("garden dev")}.`))
quit()
}
Copy link
Contributor

@Orzelius Orzelius May 11, 2023

Choose a reason for hiding this comment

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

  1. ctrl-c still exits gracefully
  2. I think this should be a bit shorter and use garden.emitwarning to it can be muted
  3. I think this should be a single line/message

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. ctrl-c still exits gracefully

It doesn't allow us enough time to guarantee that the syncs are turned off (it often is able to, but not always).

  1. I think this should be a bit shorter and use garden.emitwarning to it can be muted
  2. I think this should be a single line/message

Yeah, makes sense. I'll make those changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about now?

Copy link
Contributor

@Orzelius Orzelius May 11, 2023

Choose a reason for hiding this comment

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

Syncs started during this session may still be activ

Do we actually attempt to close them? I think it'd be cool if we didn't with ctr-c as that would allow to optionally keep them running.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be open to having that discussion, but if there are no more comments I think we should merge this one (there are a lot of improvements and new things in there) and add any more tweaks in a follow-up PR.

Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@thsig thsig merged commit 8b7cb11 into 0.13 May 12, 2023
@thsig thsig deleted the garden-up branch May 12, 2023 12:15
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.

5 participants