-
Notifications
You must be signed in to change notification settings - Fork 114
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
False positive quicksilver status on passthru #169
Comments
Hi @marfillaster thank you for opening this issue! I'm sorry, I'm not sure I understand the issue here - could you walk me through the false positive error that occurs? My understanding of the |
Hi Fatima,
Some customers are using passthru to invoke drush/wp cli commands in
quicksilver during deployment. A failed run can result to adverse effect.
Examples below:
- drush updatedb - failed db migration means possible missing column
- wp search replace - failed run means possible inclusion of platform
domain urls in search engine results
Without checking the result, there's no early feedback mechanism that would
informed customer that something went wrong during the deploy.
…On Fri, 15 May 2020, 03:56 Fatima Sarah Khalid, ***@***.***> wrote:
Hi @marfillaster <https://github.com/marfillaster> thank you for opening
this issue! I'm sorry, I'm not sure I understand the issue here - could you
walk me through the false positive error that occurs?
My understanding of the passthru command was that we don't need to worry
about the return value because we're just outputting to the browser.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#169 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA5XQL7LZDZ63JBCXEBOFDRRREHFANCNFSM4M2LGKPQ>
.
|
Oh I see, thank you for the explanation and the examples - that's very helpful. I'm going to go ahead and see if I can test your recommended solution with some of these examples and then we can open a PR to get these usages updated. cc @populist who's been working on maintaining this repo with me in case you have thoughts on the |
I definitely like the idea of providing examples that provide better error handling to folks who use them. Did a quick test of the passthru() method with our config_import script and can see errors surfaced in the output: Fatal error: Command "drush config-import-error -y" exit status: 1 in /srv/bindings/c79d510f61594387817f1360626c86dd/code/private/scripts/drush_config_import/drush_config_import.php on line 7 |
Thank you for looking into this and testing @populist! I've been reading up on If we're having issues catching non-zero/non-binary exit codes, perhaps we shouldn't be using Instead, I'd like to replace the command with something else that supports nonzero output instead of adding a layer to the existing code - maybe with the Let me know what you think :) |
passthru takes an optional second parameter to capture the status result code (0-255). The Terminus Build Tools plugin defines a convenience wrapper for passthru:
I simplified it slightly for comprehensibility. We could provide some utility traits in the examples so that users who copy them verbatim would get correct behavior. It would be desirable if the examples were correct, even though it is not entirely clear whether they were intended to ever be anything more than starting points for development. |
I received an update from Ken that the 500 timeout error is actually a client error and that engineering is looking into it. Thanks for the example @greg-1-anderson - I like the idea of the convenience wrapper or a utility trait that expands on |
I think that a good example:
I therefore think that duplicating a short passthru utility in every example is fine. |
If we start having a large number of helper functions / classes in each example, though, then the "is clear" part starts to become degraded. If this happens, then we should consider the best way to introduce dependencies to reduce duplication. |
Issue Description:
The passthru usages will result in a false positive if the called command returns with non-zero exit code.
Suggested Resolution:
Check &$return_var value and raise an error to propagate the error to the calling quicksilver script.
The text was updated successfully, but these errors were encountered: