PLAT-1243 - add data studio command stop; refactor to be able to pass in studio and datalinks by name #478
Conversation
georgi-seqera
requested review from
weronikasosnowskaseqera,
t0randr and
endre-seqera
| public List<String> mountDataIds; | ||
|
|
||
| @CommandLine.Option(names = {"--mount-data-resource-refs"}, description = "Optional configuration override for 'mountData' setting (comma separate list of data-link resource refs)", split = ",") | ||
| public List<String> mountDataResourceRefs; |
There was a problem hiding this comment.
Here I was toying with the idea of just having --mount-data and being able to pass in either the DataLink name or resource ref there and have logic to check if the string contains s3:// or az:// or even just :// and infer from that it is a resourceRef, rather than dataLink name.
It might be nicer but I don't know if wouldn't be a bit brittle, so have kept it separate params
There was a problem hiding this comment.
I think it's better to stick to separate parameters, as you did. We are going to expand the supported datalinks provider in the future, and new ones might have a different shape for the resourceRef.
| import io.seqera.tower.model.DataStudioDto; | ||
| import io.seqera.tower.model.DataStudioProgressStep; | ||
|
|
||
| import static io.seqera.tower.model.DataStudioProgressStepStatus.ERRORED; | ||
| import static io.seqera.tower.model.DataStudioProgressStepStatus.IN_PROGRESS; | ||
|
|
||
| public class AbstractStudiosCmd extends AbstractApiCmd { | ||
| public class AbstractStudiosCmd extends AbstractDataLinkCmd { |
There was a problem hiding this comment.
This reads a bit weird, but the inheritance structure makes it a bit awkward to share logic without dumping everything in the AbstractApiCmd which is a pattern I feel like is to be avoided..
There was a problem hiding this comment.
Why don't you extract the functionality that you need from AbstractDataLinkCmd and use composition instead?
There was a problem hiding this comment.
This comment was duplicated somehow :)
| import io.seqera.tower.model.DataStudioDto; | ||
| import io.seqera.tower.model.DataStudioProgressStep; | ||
|
|
||
| import static io.seqera.tower.model.DataStudioProgressStepStatus.ERRORED; | ||
| import static io.seqera.tower.model.DataStudioProgressStepStatus.IN_PROGRESS; | ||
|
|
||
| public class AbstractStudiosCmd extends AbstractApiCmd { | ||
| public class AbstractStudiosCmd extends AbstractDataLinkCmd { |
There was a problem hiding this comment.
Why don't you extract the functionality that you need from AbstractDataLinkCmd and use composition instead?
|
Does stop command support the |
| String sessionId = dataStudioRefOptions.dataStudio.sessionId != null | ||
| ? dataStudioRefOptions.dataStudio.sessionId | ||
| : fetchDataStudio(dataStudioRefOptions, wspId).getSessionId(); |
There was a problem hiding this comment.
suggestion: this feels like will be a commonly needed functionality, getting the sessionId - so maybe this could be moved to the AbstractStudiosCmd, or inside the DataStudioRefOptions?
…udio by name, resourceRef or id
georgi-seqera
force-pushed
the
PLAT-1243-add-data-studio-command-stop
branch
from
da2a534 to
30e2dc5
Compare
Context
Work relates to task: https://seqera.atlassian.net/browse/PLAT-1243
Summary: add the ability to stop a data studio via the CLI.
Testing
Trigger a stop:
Trigger a start with
--mount-datasupplied with name:Trigger a start with
--mount-data-resource-refsupplied with resourceReference:Trigger a start with
--mount-datasupplied with name where multiple datalinks match throws an error:Given we have multiple datalinks with same name:
The above is open to suggestion about how to alternatively handle this scenario - for the same case when trying to launch a pipeline by name and multiple are with same matching name are found a similar error is thrown.