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

Optionally make clever deploy no-op (without failure) or trigger a restart if repo is up-to-date. #422

Closed
clementd-fretlink opened this issue Jun 19, 2020 · 6 comments
Labels
command:deploy Tasks related to the deploy command

Comments

@clementd-fretlink
Copy link
Contributor

We recently switched from git push to clever deploy now that it returns correctly.

When running clever deploy on an up-to-date application, this triggers an error, telling us to use clever restart. So when we run the deployment job twice, it fails, instead of silently no-oping as before.

Parsing the output of clever deploy in case of error is not really desirable, so we could instead run clever status before, and chose what to do based on the deployed commit id. This would involve ugly and brittle bashisms.

A nice solution would be instead to add a flag to clever deploy, to instruct it to trigger a restart if the application is up-to-date (for completion sake, it would be also be nice to ask it to noop and return successfully as well).

For users, failing and displaying "run clever restart" is nice. For CIs, it's not super convenient.

as always, I'd be happy to take care of the implementation if needed.

@clementd-fretlink clementd-fretlink changed the title Optionally make clever deploy trigger a restart if repo is up-to-date. Optionally make clever deploy no-op (without failure) or trigger a restart if repo is up-to-date. Jul 1, 2020
@clementd-fretlink
Copy link
Contributor Author

Here's an update on the situation:

I gave up and wrote a script that runs clever status beforehand. This allows to handle the case where the running commit is the commit the CI wants to deploy.

However, it does not handle the more annoying case where the running commit is not the commit the CI wants to deploy (but the commit the CI wants to deploy has already been pushed). This can happen when we rollback or when a deployment fails.

The simplest solution here would be to grep the output of clever deploy in case of errors. The current behaviour is fine for users, but error messages suggesting other actions are near useless in scripting / CI setups.

@jtanguy
Copy link
Contributor

jtanguy commented Sep 22, 2020

I am having this exact problem. I am willing to submit a PR to fix this, but I see two options here

  1. Create a new plumbing command deploy-or-restart, which deploys or restarts without cache (with maybe a flag to allow the cache)
  2. Interpret the --force flag differently by also restarting without the cache if the commit is the same

Any thoughts on this ?

@divarvel
Copy link
Contributor

divarvel commented Sep 22, 2020

The minimal change would be for clever deploy to fail with a specific error code if the code is alreay there so that the script can retry with clever restart without having to resort to parsing stderr.

Overloading --force could make sense, but I'm not sure about why it should affect the cache as well (affecting the application cache, why not, but disabling dependencies cache would cause issues).

In any case (either with a deploy-or-restart command, or with a --or-restart flag) it would mix options from both commands, so a bit of design work may be a good idea (discussing the semantics of all the possible option combinations)

@jtanguy
Copy link
Contributor

jtanguy commented Oct 1, 2020

Any opinion @hsablonniere ? I will gladly implement this feature, but I do not want to impose an unwanted design for this feature

@hsablonniere
Copy link
Member

Sorry, I'm a bit overwhelmed by other projects. I should have answer sooner.
I will have a look in the following weeks.

@aurrelhebert aurrelhebert added the command:deploy Tasks related to the deploy command label Jul 25, 2023
@hsablonniere
Copy link
Member

@clementd-fretlink @divarvel @jtanguy We're putting the project back on tracks and proposed a solution for this problem in #276, let's discuss the details there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:deploy Tasks related to the deploy command
Projects
None yet
Development

No branches or pull requests

5 participants