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.

Sign up for GitHub

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

Jump to bottom

TSPS-325 Support for adding more CLI commands #164

Merged
merged 4 commits into from
Nov 25, 2024
Merged

TSPS-325 Support for adding more CLI commands #164

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
332e62b
fix required fields in PipelineRun
mmorgantaylor Nov 21, 2024
10079ca
add timestamps to pipeline run result
mmorgantaylor Nov 21, 2024
18c67ed
add to test
mmorgantaylor Nov 21, 2024
4f5cabf
update api object description
mmorgantaylor Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion common/openapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,14 +733,18 @@ components:
description: |
Object containing the job id, status, and user-provided description of a Pipeline Run.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can update this description

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call thanks!

type: object
required: [ pipelineName, displayName, description ]
required: [ jobId, status, timeSubmitted ]
properties:
jobId:
$ref: '#/components/schemas/Id'
status:
$ref: "#/components/schemas/PipelineRunStatus"
description:
$ref: "#/components/schemas/PipelineRunDescription"
timeSubmitted:
$ref: "#/components/schemas/JobTimeSubmitted"
timeCompleted:
$ref: "#/components/schemas/JobTimeCompleted"

PipelineRunDescription:
description: |
Expand Down
7 changes: 6 additions & 1 deletion service/src/main/java/bio/terra/pipelines/app/controller/PipelineRunsApiController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,12 @@ public ResponseEntity<ApiGetPipelineRunsResponse> getAllPipelineRuns(
new ApiPipelineRun()
.jobId(pipelineRun.getJobId())
.status(pipelineRun.getStatus().name())
.description(pipelineRun.getDescription()))
.description(pipelineRun.getDescription())
.timeSubmitted(pipelineRun.getCreated().toString())
.timeCompleted(
pipelineRun.getStatus().isCompleted()
? pipelineRun.getUpdated().toString()
: null))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does setting it to null change the default behavior of not being in the response if there is no value? I'm pretty sure it doesnt since its starts out as null but just curious

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has the same effect as just not setting timeCompleted at all if the run isn't completed. here's what's in the API response:

    {
      "jobId": "30d0ec3d-06e0-41ef-944a-846704926037",
      "status": "RUNNING",
      "description": "local test jobmapkeys refactor and hooks, fixed wdl version",
      "timeSubmitted": "2024-10-08T13:34:39.289930"
    },

do you prefer that we not set it, rather than set it to null? it seems like it might make the logic a little gnarlier since I don't know if you can call a .timeCompleted() inside an if statement?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope this is great, i was just double checking the response was the same even though it should have been obvious to me that it was

.toList();

ApiGetPipelineRunsResponse apiGetPipelineRunsResponse =
Expand Down
4 changes: 4 additions & 0 deletions service/src/main/java/bio/terra/pipelines/common/utils/CommonPipelineRunStatusEnum.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,8 @@ public enum CommonPipelineRunStatusEnum {
public boolean isSuccess() {
return this == SUCCEEDED;
}

public boolean isCompleted() {
return this == SUCCEEDED || this == FAILED;
}
}
45 changes: 37 additions & 8 deletions service/src/test/java/bio/terra/pipelines/controller/PipelineRunsApiControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,9 +585,13 @@ void getAsyncResultEndpointHttp() {
void getAllPipelineRunsWithNoPageToken() throws Exception {
int limit = 5;
String pageToken = null;
PipelineRun pipelineRun = getPipelineRunPreparing();
PipelineRun pipelineRunPreparing = getPipelineRunPreparing();
PipelineRun pipelineRunSucceeded =
getPipelineRunWithStatus(CommonPipelineRunStatusEnum.SUCCEEDED);
PipelineRun pipelineRunFailed = getPipelineRunWithStatus(CommonPipelineRunStatusEnum.FAILED);
PageResponse<List<PipelineRun>> pageResponse =
new PageResponse<>(List.of(pipelineRun), null, null);
new PageResponse<>(
List.of(pipelineRunPreparing, pipelineRunSucceeded, pipelineRunFailed), null, null);

// the mocks
when(pipelineRunsServiceMock.findPipelineRunsPaginated(
Expand All @@ -605,13 +609,38 @@ void getAllPipelineRunsWithNoPageToken() throws Exception {
new ObjectMapper()
.readValue(result.getResponse().getContentAsString(), ApiGetPipelineRunsResponse.class);

// response should include one pipeline run
// response should include three pipeline runs
assertNull(response.getPageToken());
assertEquals(1, response.getResults().size());
ApiPipelineRun responsePipelineRun = response.getResults().get(0);
assertEquals(pipelineRun.getStatus().name(), responsePipelineRun.getStatus());
assertEquals(pipelineRun.getDescription(), responsePipelineRun.getDescription());
assertEquals(pipelineRun.getJobId(), responsePipelineRun.getJobId());
assertEquals(3, response.getResults().size());

// preparing run should not have a completed time
ApiPipelineRun responsePipelineRun1 = response.getResults().get(0);
assertEquals(pipelineRunPreparing.getStatus().name(), responsePipelineRun1.getStatus());
assertEquals(pipelineRunPreparing.getDescription(), responsePipelineRun1.getDescription());
assertEquals(pipelineRunPreparing.getJobId(), responsePipelineRun1.getJobId());
assertEquals(
pipelineRunPreparing.getCreated().toString(), responsePipelineRun1.getTimeSubmitted());
assertNull(responsePipelineRun1.getTimeCompleted());

// succeeded run should have a completed time
ApiPipelineRun responsePipelineRun2 = response.getResults().get(1);
assertEquals(pipelineRunSucceeded.getStatus().name(), responsePipelineRun2.getStatus());
assertEquals(pipelineRunSucceeded.getDescription(), responsePipelineRun2.getDescription());
assertEquals(pipelineRunSucceeded.getJobId(), responsePipelineRun2.getJobId());
assertEquals(
pipelineRunSucceeded.getCreated().toString(), responsePipelineRun2.getTimeSubmitted());
assertEquals(
pipelineRunSucceeded.getUpdated().toString(), responsePipelineRun2.getTimeCompleted());

// failed run should have a completed time
ApiPipelineRun responsePipelineRun3 = response.getResults().get(2);
assertEquals(pipelineRunFailed.getStatus().name(), responsePipelineRun3.getStatus());
assertEquals(pipelineRunFailed.getDescription(), responsePipelineRun3.getDescription());
assertEquals(pipelineRunFailed.getJobId(), responsePipelineRun3.getJobId());
assertEquals(
pipelineRunFailed.getCreated().toString(), responsePipelineRun3.getTimeSubmitted());
assertEquals(
pipelineRunFailed.getUpdated().toString(), responsePipelineRun3.getTimeCompleted());
}

@Test
Expand Down
Loading