Add extra metadata to workflow runs exported files #364
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,11 +24,15 @@ | |
| import io.seqera.tower.cli.exceptions.TowerException; | ||
| import io.seqera.tower.cli.responses.Response; | ||
| import io.seqera.tower.cli.responses.runs.RunDump; | ||
| import io.seqera.tower.cli.shared.WorkflowMetadata; | ||
| import io.seqera.tower.cli.utils.SilentPrintWriter; | ||
| import io.seqera.tower.model.DescribeTaskResponse; | ||
| import io.seqera.tower.model.DescribeWorkflowLaunchResponse; | ||
| import io.seqera.tower.model.DescribeWorkflowResponse; | ||
| import io.seqera.tower.model.Launch; | ||
| import io.seqera.tower.model.ListParticipantsResponse; | ||
| import io.seqera.tower.model.ListTasksResponse; | ||
| import io.seqera.tower.model.ParticipantDbDto; | ||
| import io.seqera.tower.model.ServiceInfo; | ||
| import io.seqera.tower.model.Task; | ||
| import io.seqera.tower.model.TaskStatus; | ||
|
|
@@ -167,15 +171,47 @@ private void dumpTasks(PrintWriter progress, TarArchiveOutputStream out, Long ws | |
| private void dumpWorkflowDetails(PrintWriter progress, TarArchiveOutputStream out, Long wspId) throws ApiException, IOException { | ||
| progress.println(ansi("- Workflow details")); | ||
|
|
||
| // General workflow info | ||
| DescribeWorkflowResponse workflowResponse = workflowById(wspId, id); | ||
| Workflow workflow = workflowResponse.getWorkflow(); | ||
| if (workflow == null) { | ||
| throw new TowerException("Unknown workflow"); | ||
| } | ||
|
|
||
| // Launch info | ||
| Launch launch = null; | ||
| Long pipelineId = null; | ||
| if (workflow.getLaunchId() != null) { | ||
|
|
||
| launch = launchById(wspId, workflow.getLaunchId()); | ||
|
|
||
| DescribeWorkflowLaunchResponse wfLaunchResponse = workflowLaunchById(wspId, workflow.getId()); | ||
| if (wfLaunchResponse != null && wfLaunchResponse.getLaunch() != null) { | ||
|
There was a problem hiding this comment. Same here. What is the "if" checking? I think it would be better to create a separate method that returns a boolean, and has a descriptive name of what we are actually checking. Example: There was a problem hiding this comment. I think the |
||
| pipelineId = wfLaunchResponse.getLaunch().getPipelineId(); | ||
| } | ||
| } | ||
|
|
||
| // User info | ||
| Long userId = workflow.getOwnerId(); | ||
| String userMail = null; | ||
| String userName = workflow.getUserName(); | ||
| try { | ||
|
There was a problem hiding this comment. Same here. This Try catch could be hidden on a separate method with a descriptive name. That way all this method becomes way simpler and easy to understand. There was a problem hiding this comment. I think that moving fragments of the code to a separate method makes sense if said method is getting reused more than once or if the fragment is overly complex. Otherwise we are just scattering the code around without actually reducing complexity. There was a problem hiding this comment. I think that there are other advantages in splitting the code in smaller methods, rather than just for code reuse:
I will give you an example: Both methods that were extracted are only used once, but in your opinion: There was a problem hiding this comment. Comments are not pollution in the code base. We strongly encourage adding comments wherever is necessary as long as they explain the "why" rather than the "what" in the code.
This is subjective, sure the wrapper method is 3 lines but then you have moved the same code into two methods, so you end with N+3 lines and additional jumps that makes debugging more difficult. P.S: Thanks a lot for the recommendation, I enjoyed reading that book back in the day. Let me return the favor and recommend you another book. There was a problem hiding this comment.
Not really, what happens is that both methods will call a single method where all the duplicate code is. However, that part is not shown in the screenshot.
Thanks for the book recommendation 👍🏻 |
||
| ListParticipantsResponse participants = api().listWorkspaceParticipants(workflowResponse.getOrgId(), workflowResponse.getWorkspaceId(), null, null, null); | ||
JaimeSeqLabs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| userMail = participants.getParticipants().stream() | ||
| .filter(participant -> userName.equals(participant.getUserName())) | ||
| .findFirst() | ||
| .map(ParticipantDbDto::getEmail) | ||
| .orElse(null); | ||
| } catch (ApiException ignored) {} | ||
|
|
||
| // Load and metrics info | ||
| WorkflowLoad workflowLoad = workflowLoadByWorkflowId(wspId, id); | ||
| List<WorkflowMetrics> metrics = api().describeWorkflowMetrics(workflow.getId(), wspId).getMetrics(); | ||
|
|
||
| WorkflowMetadata wfMetadata = new WorkflowMetadata(pipelineId, wspId, userId, userMail); | ||
|
|
||
| addEntry(out, "workflow.json", Workflow.class, workflow); | ||
| addEntry(out, "workflow-metadata.json", WorkflowMetadata.class, wfMetadata); | ||
| addEntry(out, "workflow-load.json", WorkflowLoad.class, workflowLoad); | ||
| addEntry(out, "workflow-launch.json", Launch.class, launch); | ||
| addEntry(out, "workflow-metrics.json", List.class, metrics); | ||
|
|
@@ -285,8 +321,14 @@ private <T> void addEntry(TarArchiveOutputStream out, String fileName, Class<T> | |
| if (value == null) { | ||
| return; | ||
| } | ||
| addEntry(out, fileName, toJSON(type, value)); | ||
| } | ||
|
|
||
| private void addEntry(TarArchiveOutputStream out, String fileName, byte[] data) throws IOException { | ||
| if (data == null) { | ||
| return; | ||
| } | ||
| TarArchiveEntry entry = new TarArchiveEntry(fileName); | ||
| entry.setSize(data.length); | ||
| out.putArchiveEntry(entry); | ||
| out.write(data); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we create a separate method with a descriptive name of what is happening here, instead of directly put the if clause?