Provide support for deferred services#49
Conversation
| } | ||
|
|
||
| if s.Deferred { | ||
| // make sure all the deferred dependencies are started |
There was a problem hiding this comment.
this logic is a bit confusing, aren't we skipping the deferred deps on line 83? Taking a step back, should launching a service for all deps (even deferred) to start?
Also, what happens if a non-deferred service depends on a deferred one? Should we make that an analysis time error?
There was a problem hiding this comment.
When you start a deferred service through the http API, the contract is that all deferred services should also be manually started though. The comment is actually the opposite, it should relate to non-deferred services...
And yeah, we should be able to do some checks at analysis time, I'll check that out
| @@ -0,0 +1,93 @@ | |||
| package svcctl | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
we can provide this as an official client package in the main module, its pretty small but nice to have and adds no deps
There was a problem hiding this comment.
The only reason I didn't provide that as an official API, is that it's golang based and very simple. Also most of our integration tests are run from python code so it wasn't even necessary for us.
I don't have any objection though
There was a problem hiding this comment.
ok we can just keep it here for now and maybe move as followup
| httpClient *http.Client | ||
| } | ||
|
|
||
| func NewSvcctlClient(baseURL string, client *http.Client) *SvcctlClient { |
There was a problem hiding this comment.
if we make this official API, should we grab the baseURL from the env var by default to make it simpler?
There was a problem hiding this comment.
What would you see ? a config object that we would pass to the New client function rather than all the required parameters ?
Then we can populate that object with proper defaults.
darkrift
force-pushed
the
issue38_take2
branch
2 times, most recently
from
ebdb938 to
b7183e8
Compare
darkrift
force-pushed
the
issue38_take2
branch
from
b7183e8 to
eb5b51e
Compare
| RunEnvironmentInfo(environment = _run_environment(ctx, service_specs_file)), | ||
| DefaultInfo(runfiles = runfiles), | ||
| _ServiceGroupInfo(services = services), | ||
| _ServiceGroupInfo(services = services, deferred = False), |
There was a problem hiding this comment.
i think group also needs a deferred prop, otherwise you can have (non-deferred service) depend on (group) depend on (deferred service) and it will defeat the check you added
There was a problem hiding this comment.
same thing with tasks, right? let's just add the attribute on all of them
2 participants
This allow services to be deferred upon start meaning the runner will not automatically start them. One need to use the svcctl endpoint to start the service.
This provides support to declare services that needs to be delayed for the test environment to be setup first and then started such as cron jobs.
This also adds support to properly pass TMPDIR and SOCKET_DIR as env vars to the test