fix: add a retry for unsetting environment variable in content_restart()#499
fix: add a retry for unsetting environment variable in content_restart()#499
content_restart()#499Conversation
R/content.R
Outdated
| Sys.sleep(1) | ||
| content$environment_set("{env_var_name}" := NA) | ||
| } else { | ||
| stop(e) |
There was a problem hiding this comment.
I'm fine either way, but I wonder if this should be a warning rather than a stop? It is a bit of a funny error, but like we see here, the thing worked, it just hasn't totally cleaned itself up.
There was a problem hiding this comment.
yeah, warning makes sense, I'll update it.
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
I'm approving, but one comment of stop versus warning.
Also, given the server-state stuff httptest might be harder to use, but in other places I wonder if we could get away from maintaining this separate mock connect infrastructure and just lean into httptest — thoughts?
To be honest I find the mock connect class way easier to work with and reason about than the httptest tests so I'm not terribly excited about that. |
2 participants
Intent
content_restart()sets and immediately unsets an environment variable to restart content on Connect. However sometimes the unsetting is not working, leading to a proliferation of env vars. I believe that in these cases the error is on the server side andcontent_restart()fails with a 500. As a workaround, this PR adds atryCatchthat will retry the request a single time if it gets a 500 error.Closes #498
Checklist
NEWS.md(referencing the connected issue if necessary)?devtools::document()?