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
Restart rework #1457
Restart rework #1457
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see the machines stuff being split out! Just left some comments.
internal/command/apps/restart.go
Outdated
) | ||
|
||
func newRestart() *cobra.Command { | ||
const ( | ||
long = `The APPS RESTART command will restart all running vms. | ||
` | ||
long = `The APPS RESTART command will perform a rolling restart against all running VM's` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just be "VMs" here
NewOpen(), | ||
NewReleases(), | ||
) | ||
|
||
return apps | ||
} | ||
|
||
// BuildContext is a helper that builds out commonly required context requirements | ||
func BuildContext(ctx context.Context, app *api.AppCompact) (context.Context, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mainly to get the flaps client into context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the flaps client and dialer.
@@ -0,0 +1,52 @@ | |||
package machine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a problem that we have the machines package split across internal/command/machine
and internal/machine
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename the package if it's confusing, but it's not really an issue afaict. If both packages need to be used at the same time we will just need to define an alias during import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someday we can get rid of that platform version if conditional
Notable changes:
fly apps restart now supports Machines.
Machine orchestration primitives have been abstracted out into a higher level
machines
package. This will allow us to easily re-use them across command packages.https://github.com/superfly/flyctl/tree/fly-restart-rework-2/internal/machine
fly restart
This command is being softly removed. A user will receive an error indicating that they should use
fly apps restart
instead.fly apps restart
This is what almost all apps should be using.
Unfortunately, I was not able to include the PG restart process due to a cyclic dependency issue. The postgres package will need to be restructured a bit before we can make this work. PG apps will receive an error when hitting this command indicating they should use
fly pg restart
instead. Not great, but progress.fly pg restart
Has been reworked to use the new machine orchestration primitives.
fly machine restart
Has been reworked to use the new machine orchestration primitives.