-
Notifications
You must be signed in to change notification settings - Fork 107
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
WIP: Make rebooting a flag for "ob deploy push" #1036
base: master
Are you sure you want to change the base?
Conversation
echo "${bashEscape (show reboot)}" | ||
if [[ "$booted" = "$built" && "${bashEscape (show reboot)}" == "${bashEscape (show False)}" ]]; then |
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.
It seems to me like it might be better to just template in the reboot if the flag is set rather than doing logic in bash
{-# LANGUAGE ScopedTypeVariables #-} | ||
{-# LANGUAGE TupleSections #-} | ||
{-# LANGUAGE PackageImports #-} | ||
{- ORMOLU_DISABLE -} |
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.
I don't think we should include formatting app specific pragmas.
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.
Agree
@@ -154,6 +155,9 @@ deployCommand cfg = hsubparser $ mconcat | |||
, command "ios" $ info ((,) <$> pure IOS <*> fmap pure (strArgument (metavar "TEAMID" <> help "Your Team ID - found in the Apple developer portal"))) mempty | |||
] | |||
|
|||
reboot :: Parser Bool | |||
reboot = flag False True (long "reboot") |
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.
Rebooting on deploy is currently the default behaviour, this shouldn't change that. Otherwise users may be surprised to see that their running apps aren't the latest version following a deployment.
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.
I'd argue that it should've never been turned on by default in the first place, This should've always been the users choice to actually reboot
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.
You're talking about restarting the backend service, right?
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.
I was mistaken this is referring to the restart of the machine the backend service is running on. The backend service will still be restarted as part of a deploy that did not include the --reboot
flag.
Any thoughts on whether this will be merged? I'm currently holding off on updating to latest obelisk and using a branch with this patch. |
We might not always want to reboot the remote server, this should be the users decision to actually reboot
I have:
develop
branchhlint .
(lint found code you did not write can be left alone)$(nix-build -A selftest --no-out-link)
nix-build release.nix -A build.x86_64-linux --no-out-link
(orx86_64-darwin
on macOS)