-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
cli: implement diff
command
#82
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
/* | ||
This file is part of REANA. | ||
Copyright (C) 2022 CERN. | ||
|
||
REANA is free software; you can redistribute it and/or modify it | ||
under the terms of the MIT License; see LICENSE file for more details. | ||
*/ | ||
|
||
package cmd | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"io" | ||
"reanahub/reana-client-go/client" | ||
"reanahub/reana-client-go/client/operations" | ||
"reanahub/reana-client-go/utils" | ||
|
||
"github.com/jedib0t/go-pretty/v6/text" | ||
|
||
"github.com/spf13/cobra" | ||
) | ||
|
||
const diffDesc = ` | ||
Show diff between two workflows. | ||
|
||
The ` + "``diff``" + ` command allows to compare two workflows, the workflow_a and | ||
workflow_b, which must be provided as arguments. The output will show the | ||
difference in workflow run parameters, the generated files, the logs, etc. | ||
|
||
Examples: | ||
|
||
$ reana-client diff myanalysis.42 myotheranalysis.43 | ||
|
||
$ reana-client diff myanalysis.42 myotheranalysis.43 --brief | ||
` | ||
|
||
type diffOptions struct { | ||
token string | ||
workflowA string | ||
workflowB string | ||
brief bool | ||
unified int | ||
} | ||
|
||
// newDiffCmd creates a command to show diff between two workflows. | ||
func newDiffCmd() *cobra.Command { | ||
o := &diffOptions{} | ||
|
||
cmd := &cobra.Command{ | ||
Use: "diff", | ||
Short: "Show diff between two workflows.", | ||
Long: diffDesc, | ||
Args: cobra.ExactArgs(2), | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
o.workflowA = args[0] | ||
o.workflowB = args[1] | ||
return o.run(cmd) | ||
}, | ||
} | ||
|
||
f := cmd.Flags() | ||
f.StringVarP(&o.token, "access-token", "t", "", "Access token of the current user.") | ||
f.BoolVarP(&o.brief, "brief", "q", false, `If not set, differences in the contents of the | ||
files in the two workspaces are shown.`) | ||
f.IntVarP( | ||
&o.unified, "unified", "u", 5, "Sets number of context lines for workspace diff output.", | ||
) | ||
|
||
return cmd | ||
} | ||
|
||
func (o *diffOptions) run(cmd *cobra.Command) error { | ||
diffParams := operations.NewGetWorkflowDiffParams() | ||
diffParams.SetAccessToken(&o.token) | ||
diffParams.SetWorkflowIDOrNamea(o.workflowA) | ||
diffParams.SetWorkflowIDOrNameb(o.workflowB) | ||
diffParams.SetBrief(&o.brief) | ||
contextLines := fmt.Sprintf("%d", o.unified) | ||
diffParams.SetContextLines(&contextLines) | ||
|
||
api, err := client.ApiClient() | ||
if err != nil { | ||
return err | ||
} | ||
diffResp, err := api.Operations.GetWorkflowDiff(diffParams) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
err = displayDiffPayload(cmd, diffResp.Payload) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func displayDiffPayload(cmd *cobra.Command, p *operations.GetWorkflowDiffOKBody) error { | ||
leadingMark := "==>" | ||
|
||
if p.ReanaSpecification != "" { | ||
var specificationDiff map[string][]string | ||
err := json.Unmarshal([]byte(p.ReanaSpecification), &specificationDiff) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// Rename section workflow to specification | ||
val, hasWorkflow := specificationDiff["workflow"] | ||
if hasWorkflow { | ||
specificationDiff["specification"] = val | ||
delete(specificationDiff, "workflow") | ||
} | ||
equalSpecification := true | ||
for section, lines := range specificationDiff { | ||
if len(lines) != 0 { | ||
equalSpecification = false | ||
utils.PrintColorable( | ||
fmt.Sprintf("%s Differences in workflow %s\n", leadingMark, section), | ||
cmd.OutOrStdout(), | ||
text.FgYellow, | ||
text.Bold, | ||
) | ||
printDiff(lines, cmd.OutOrStdout()) | ||
} | ||
} | ||
if equalSpecification { | ||
utils.PrintColorable( | ||
fmt.Sprintf("%s No differences in REANA specifications.\n", leadingMark), | ||
cmd.OutOrStdout(), | ||
text.FgYellow, | ||
text.Bold, | ||
) | ||
} | ||
cmd.Println() // Separation line | ||
} | ||
|
||
var workspaceDiffRaw string | ||
err := json.Unmarshal([]byte(p.WorkspaceListing), &workspaceDiffRaw) | ||
if err != nil { | ||
return err | ||
} | ||
if workspaceDiffRaw != "" { | ||
workspaceDiff := utils.SplitLinesNoEmpty(workspaceDiffRaw) | ||
|
||
utils.PrintColorable( | ||
fmt.Sprintf("%s Differences in workflow workspace\n", leadingMark), | ||
cmd.OutOrStdout(), | ||
text.FgYellow, | ||
text.Bold, | ||
) | ||
printDiff(workspaceDiff, cmd.OutOrStdout()) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func printDiff(lines []string, out io.Writer) { | ||
for _, line := range lines { | ||
lineColor := text.Reset | ||
switch line[0] { | ||
case '@': | ||
lineColor = text.FgCyan | ||
case '-': | ||
lineColor = text.FgRed | ||
case '+': | ||
lineColor = text.FgGreen | ||
} | ||
|
||
utils.PrintColorable(line, out, lineColor) | ||
fmt.Fprintln(out) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
/* | ||
This file is part of REANA. | ||
Copyright (C) 2022 CERN. | ||
|
||
REANA is free software; you can redistribute it and/or modify it | ||
under the terms of the MIT License; see LICENSE file for more details. | ||
*/ | ||
|
||
package cmd | ||
|
||
import ( | ||
"bytes" | ||
"fmt" | ||
"net/http" | ||
"reanahub/reana-client-go/utils" | ||
"testing" | ||
|
||
"github.com/jedib0t/go-pretty/v6/text" | ||
) | ||
|
||
var diffPathTemplate = "/api/workflows/%s/diff/%s" | ||
|
||
func TestDiff(t *testing.T) { | ||
workflowA := "my_workflow_a" | ||
workflowB := "my_workflow_b" | ||
diffResponse := `{ | ||
"reana_specification":` + `"{` + | ||
`\"version\": [\"@@ -1 +1 @@\", \"- v0.1\", \"+ v0.2\"],` + | ||
`\"inputs\": [\"@@ -1 +2 @@\", \"- removed input\", \"+ added input\", \"+ more input\"],` + | ||
`\"outputs\": [\"@@ -2 +1 @@\", \"- removed output\", \"- more output\", \"+ added output\"],` + | ||
`\"workflow\": [\"@@ +1 @@\", \"+ added specs\"]` + | ||
`}"` + `, | ||
"workspace_listing": "\"Only in my_workflow_a: test.yaml\"" | ||
}` | ||
sameSpecResponse := `{ | ||
"reana_specification":` + `"{` + | ||
`\"version\": [],\"inputs\": [],\"outputs\": [],\"specification\": []` + | ||
`}"` + `, | ||
"workspace_listing": "\"Only in my_workflow_a: test.yaml\"" | ||
}` | ||
noSpecResponse := `{ | ||
"reana_specification": "", | ||
"workspace_listing": "\"Only in my_workflow_a: test.yaml\"" | ||
}` | ||
noWorkspaceResponse := `{ | ||
"reana_specification":` + `"{` + | ||
`\"version\": [],\"inputs\": [],\"outputs\": [],\"specification\": []` + | ||
`}"` + `, | ||
"workspace_listing": "\"\"" | ||
}` | ||
|
||
tests := map[string]TestCmdParams{ | ||
"all info": { | ||
serverResponse: diffResponse, | ||
statusCode: http.StatusOK, | ||
args: []string{workflowA, workflowB}, | ||
expected: []string{ | ||
"Differences in workflow version", "@@ -1 +1 @@", "- v0.1", "+ v0.2", | ||
"Differences in workflow inputs", "@@ -1 +2 @@", "- removed input", "+ added input", "+ more input", | ||
"Differences in workflow outputs", "@@ -2 +1 @@", "- removed output", "- more output", "+ added output", | ||
"Differences in workflow specification", "@@ +1 @@", "+ added specs", | ||
"Differences in workflow workspace", "Only in my_workflow_a: test.yaml", | ||
}, | ||
}, | ||
"same specification": { | ||
serverResponse: sameSpecResponse, | ||
statusCode: http.StatusOK, | ||
args: []string{workflowA, workflowB}, | ||
expected: []string{ | ||
"No differences in REANA specifications", | ||
"Differences in workflow workspace", "Only in my_workflow_a: test.yaml", | ||
}, | ||
unwanted: []string{ | ||
"Differences in workflow version", "Differences in workflow inputs", | ||
"Differences in workflow specification", "Differences in workflow outputs", | ||
}, | ||
}, | ||
"no specification info": { | ||
serverResponse: noSpecResponse, | ||
statusCode: http.StatusOK, | ||
args: []string{workflowA, workflowB}, | ||
expected: []string{ | ||
"Differences in workflow workspace", "Only in my_workflow_a: test.yaml", | ||
}, | ||
unwanted: []string{ | ||
"No differences in REANA specifications", | ||
"Differences in workflow version", "Differences in workflow inputs", | ||
"Differences in workflow specification", "Differences in workflow outputs", | ||
}, | ||
}, | ||
"no workspace info": { | ||
serverResponse: noWorkspaceResponse, | ||
statusCode: http.StatusOK, | ||
args: []string{workflowA, workflowB}, | ||
expected: []string{ | ||
"No differences in REANA specifications", | ||
}, | ||
unwanted: []string{ | ||
"Differences in workflow workspace", | ||
}, | ||
}, | ||
"unexisting workflow": { | ||
serverResponse: `{"message": "Workflow my_workflow_a does not exist."}`, | ||
statusCode: http.StatusNotFound, | ||
args: []string{workflowA, workflowB}, | ||
expected: []string{"Workflow my_workflow_a does not exist."}, | ||
wantError: true, | ||
}, | ||
"invalid number of arguments": { | ||
args: []string{workflowA}, | ||
expected: []string{"accepts 2 arg(s), received 1"}, | ||
wantError: true, | ||
}, | ||
} | ||
|
||
for name, params := range tests { | ||
t.Run(name, func(t *testing.T) { | ||
params.cmd = "diff" | ||
params.serverPath = fmt.Sprintf(diffPathTemplate, workflowA, workflowB) | ||
testCmdRun(t, params) | ||
}) | ||
} | ||
} | ||
|
||
func TestPrintDiff(t *testing.T) { | ||
tests := map[string]struct { | ||
lines []string | ||
expectedColors []text.Color | ||
}{ | ||
"default text": { | ||
lines: []string{"default text"}, | ||
expectedColors: []text.Color{text.Reset}, | ||
}, | ||
"diff info": { | ||
lines: []string{"@@ -1,14 +1,26 @@"}, | ||
expectedColors: []text.Color{text.FgCyan}, | ||
}, | ||
"removed text": { | ||
lines: []string{"- removed text"}, | ||
expectedColors: []text.Color{text.FgRed}, | ||
}, | ||
"added text": { | ||
lines: []string{"+ added text"}, | ||
expectedColors: []text.Color{text.FgGreen}, | ||
}, | ||
"mixed text": { | ||
lines: []string{"@@ -1 +1 @@", "context", "- removed text", "+ added text"}, | ||
expectedColors: []text.Color{text.FgCyan, text.Reset, text.FgRed, text.FgGreen}, | ||
}, | ||
} | ||
|
||
for name, test := range tests { | ||
t.Run(name, func(t *testing.T) { | ||
resBuf := new(bytes.Buffer) | ||
printDiff(test.lines, resBuf) | ||
result := utils.SplitLinesNoEmpty(resBuf.String()) | ||
|
||
if len(result) != len(test.lines) { | ||
t.Fatalf("Expected %d lines, got %d", len(test.lines), len(result)) | ||
} | ||
for i, line := range result { | ||
testBuf := new(bytes.Buffer) | ||
utils.PrintColorable(test.lines[i], testBuf, test.expectedColors[i]) | ||
expected := testBuf.String() | ||
if line != expected { | ||
t.Errorf("Expected %s, got %s", expected, line) | ||
} | ||
} | ||
}) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
One thing I noticed with respect to the Python client is the difference section ordering:
Some observations:
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.
The ordering is different because GO's maps are unordered, unlike in Python. It's possible to maintain order by using json.Decoder instead of the json.Unmarshal being used. The code will probably be a little bit more verbose but the change shouldn't be too difficult
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.
Shall we do it now or after package refactoring?
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.
We can do it now
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.
Turns out I was wrong about using the Decoder, it works nicely for arrays but not so much for objects, since they don't guarantee order by nature. By looking at this discussion I found some hacky workarounds but no nice solution for this, the developers don't seem to support it because one should use an array for order.
An example of a not so nice solution is using the Decoder and manually parsing the JSON, it doesn't look good at all: https://go.dev/play/p/jOXpSDpJz04
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.
I'm not sure if it's worth implementing this order (if we want order, shouldn't the server return it as an array?)
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.
Yeah, fully agreed that it would be very clean to do this on the server-side. But since it would necessitate breaking REST API response changes, it's not something for now.
Hmm, looking at the pasted workaround, it might do actually as a stop-gap solution; however it's also possible that we can live with the Python client <-> Go client output differences for this commend, since it is not probable that some clients has wrapped it into some wider scripted glue usage scenarios... I'm kind of 60:40 split on this.