review feedback

- Switch to -f/-F instead of -- (this lead to some `gh api` copypasta)
- Remove --json
- Don't parse workflow YAML if not collecting interactively
- Share GetWorkflowContent
- Fix a parsing issue
This commit is contained in:
vilmibm 2021-04-08 12:04:33 -05:00 committed by vilmibm
parent f80268d966
commit 1b5eb30575
2 changed files with 311 additions and 187 deletions

View file

@ -22,7 +22,6 @@ import (
"github.com/cli/cli/pkg/iostreams"
"github.com/cli/cli/pkg/prompt"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"gopkg.in/yaml.v3"
)
@ -35,7 +34,8 @@ type RunOptions struct {
Ref string
JSON string
InputArgs []string
MagicFields []string
RawFields []string
Prompt bool
}
@ -58,9 +58,8 @@ func NewCmdRun(f *cmdutil.Factory, runF func(*RunOptions) error) *cobra.Command
If the workflow file supports inputs, they can be specified in a few ways:
- Interactively
- As command line arguments specified after --
- via -f or -F flags
- As JSON, via STDIN
- As JSON, via --json
`),
Example: heredoc.Doc(`
# Have gh prompt you for what workflow you'd like to run
@ -73,17 +72,14 @@ func NewCmdRun(f *cmdutil.Factory, runF func(*RunOptions) error) *cobra.Command
$ gh workflow run triage.yml --ref=myBranch
# Run the workflow file 'triage.yml' with command line inputs
$ gh workflow run triage.yml -- --name=scully --greeting=hello
$ gh workflow run triage.yml -fname=scully -fgreeting=hello
# Run the workflow file 'triage.yml' with JSON via STDIN
$ echo '{"name":"scully", "greeting":"hello"}' | gh workflow run triage.yml
# Run the workflow file 'triage.yml' with JSON via --json
$ gh workflow run triage.yml --json='{"name":"scully", "greeting":"hello"}'
`),
Args: func(cmd *cobra.Command, args []string) error {
if cmd.ArgsLenAtDash() == 0 && len(args[1:]) > 0 {
return cmdutil.FlagError{Err: fmt.Errorf("workflow argument required when passing input flags")}
if len(opts.MagicFields)+len(opts.RawFields) > 0 && len(args) == 0 {
return cmdutil.FlagError{Err: fmt.Errorf("workflow argument required when passing -f or -F")}
}
return nil
},
@ -91,9 +87,10 @@ func NewCmdRun(f *cmdutil.Factory, runF func(*RunOptions) error) *cobra.Command
// support `-R, --repo` override
opts.BaseRepo = f.BaseRepo
inputFieldsPassed := len(opts.MagicFields)+len(opts.RawFields) > 0
if len(args) > 0 {
opts.Selector = args[0]
opts.InputArgs = args[1:]
} else if !opts.IO.CanPrompt() {
return &cmdutil.FlagError{Err: errors.New("workflow ID, name, or filename required when not running interactively")}
} else {
@ -101,9 +98,6 @@ func NewCmdRun(f *cmdutil.Factory, runF func(*RunOptions) error) *cobra.Command
}
if !opts.IO.IsStdinTTY() {
if opts.JSON != "" {
return errors.New("JSON can only be passed on one of STDIN or --json at a time")
}
jsonIn, err := ioutil.ReadAll(opts.IO.In)
if err != nil {
return errors.New("failed to read from STDIN")
@ -116,23 +110,71 @@ func NewCmdRun(f *cmdutil.Factory, runF func(*RunOptions) error) *cobra.Command
return &cmdutil.FlagError{Err: errors.New("workflow argument required when passing JSON")}
}
} else {
if opts.JSON != "" && len(opts.InputArgs) > 0 {
return &cmdutil.FlagError{Err: errors.New("only one of JSON or input arguments can be passed at a time")}
if opts.JSON != "" && inputFieldsPassed {
return &cmdutil.FlagError{Err: errors.New("only one of STDIN or -f/-F can be passed")}
}
}
if runF != nil {
return runF(opts)
}
return runRun(opts)
},
}
cmd.Flags().StringVarP(&opts.Ref, "ref", "r", "", "The branch or tag name which contains the version of the workflow file you'd like to run")
cmd.Flags().StringVar(&opts.JSON, "json", "", "Provide workflow inputs as JSON")
cmd.Flags().StringArrayVarP(&opts.MagicFields, "field", "F", nil, "Add a string parameter in `key=value` format, respecting @ syntax")
cmd.Flags().StringArrayVarP(&opts.RawFields, "raw-field", "f", nil, "Add a string parameter in `key=value` format")
return cmd
}
// TODO copypasta from gh api
func parseField(f string) (string, string, error) {
idx := strings.IndexRune(f, '=')
if idx == -1 {
return f, "", fmt.Errorf("field %q requires a value separated by an '=' sign", f)
}
return f[0:idx], f[idx+1:], nil
}
// TODO MODIFIED copypasta from gh api
func magicFieldValue(v string, opts RunOptions) (string, error) {
if strings.HasPrefix(v, "@") {
fileBytes, err := opts.IO.ReadUserFile(v[1:])
if err != nil {
return "", err
}
return string(fileBytes), nil
}
return v, nil
}
// TODO copypasta from gh api
func parseFields(opts RunOptions) (map[string]string, error) {
params := make(map[string]string)
for _, f := range opts.RawFields {
key, value, err := parseField(f)
if err != nil {
return params, err
}
params[key] = value
}
for _, f := range opts.MagicFields {
key, strValue, err := parseField(f)
if err != nil {
return params, err
}
value, err := magicFieldValue(strValue, opts)
if err != nil {
return params, fmt.Errorf("error parsing %q value: %w", key, err)
}
params[key] = value
}
return params, nil
}
type InputAnswer struct {
providedInputs map[string]string
}
@ -152,6 +194,48 @@ func (ia *InputAnswer) WriteAnswer(name string, value interface{}) error {
return fmt.Errorf("unexpected value type: %v", value)
}
func collectInputs(yamlContent []byte) (map[string]string, error) {
inputs, err := findInputs(yamlContent)
if err != nil {
return nil, err
}
providedInputs := map[string]string{}
if len(inputs) == 0 {
return providedInputs, nil
}
qs := []*survey.Question{}
for inputName, input := range inputs {
q := &survey.Question{
Name: inputName,
Prompt: &survey.Input{
Message: inputName,
Default: input.Default,
},
}
if input.Required {
q.Validate = survey.Required
}
qs = append(qs, q)
}
sort.Slice(qs, func(i, j int) bool {
return qs[i].Name < qs[j].Name
})
inputAnswer := InputAnswer{
providedInputs: providedInputs,
}
err = prompt.SurveyAsk(qs, &inputAnswer)
if err != nil {
return nil, err
}
return providedInputs, nil
}
func runRun(opts *RunOptions) error {
c, err := opts.HttpClient()
if err != nil {
@ -164,6 +248,15 @@ func runRun(opts *RunOptions) error {
return fmt.Errorf("could not determine base repo: %w", err)
}
ref := opts.Ref
if ref == "" {
ref, err = api.RepoDefaultBranch(client, repo)
if err != nil {
return fmt.Errorf("unable to determine default branch for %s: %w", ghrepo.FullName(repo), err)
}
}
states := []shared.WorkflowState{shared.Active}
workflow, err := shared.ResolveWorkflow(
opts.IO, client, repo, opts.Prompt, opts.Selector, states)
@ -175,81 +268,30 @@ func runRun(opts *RunOptions) error {
return err
}
ref := opts.Ref
if ref == "" {
ref, err = api.RepoDefaultBranch(client, repo)
if err != nil {
return fmt.Errorf("unable to determine default branch for %s: %w", ghrepo.FullName(repo), err)
}
}
yamlContent, err := getWorkflowContent(client, repo, workflow, ref)
if err != nil {
return fmt.Errorf("unable to fetch workflow file content: %w", err)
}
inputs, err := findInputs(yamlContent)
if err != nil {
return err
}
providedInputs := map[string]string{}
if opts.Prompt {
qs := []*survey.Question{}
for inputName, input := range inputs {
q := &survey.Question{
Name: inputName,
Prompt: &survey.Input{
Message: inputName,
Default: input.Default,
},
}
if input.Required {
q.Validate = survey.Required
}
qs = append(qs, q)
}
sort.Slice(qs, func(i, j int) bool {
return qs[i].Name < qs[j].Name
})
inputAnswer := InputAnswer{
providedInputs: providedInputs,
}
err = prompt.SurveyAsk(qs, &inputAnswer)
if len(opts.MagicFields)+len(opts.RawFields) > 0 {
providedInputs, err = parseFields(*opts)
if err != nil {
return err
}
} else {
if opts.JSON != "" {
err := json.Unmarshal([]byte(opts.JSON), &providedInputs)
if err != nil {
return fmt.Errorf("could not parse provided JSON: %w", err)
}
}
}
if len(opts.InputArgs) > 0 {
fs := pflag.FlagSet{}
for inputName, input := range inputs {
fs.String(inputName, input.Default, input.Description)
}
err = fs.Parse(opts.InputArgs)
if err != nil {
return fmt.Errorf("could not parse input args: %w", err)
}
for inputName := range inputs {
providedValue, _ := fs.GetString(inputName)
providedInputs[inputName] = providedValue
}
if opts.JSON != "" {
err := json.Unmarshal([]byte(opts.JSON), &providedInputs)
if err != nil {
return fmt.Errorf("could not parse provided JSON: %w", err)
}
}
for inputName, inputValue := range inputs {
if inputValue.Required && providedInputs[inputName] == "" {
return fmt.Errorf("missing required input '%s'", inputName)
if opts.Prompt {
yamlContent, err := getWorkflowContent(client, repo, workflow, ref)
if err != nil {
return fmt.Errorf("unable to fetch workflow file content: %w", err)
}
providedInputs, err = collectInputs(yamlContent)
if err != nil {
return err
}
}
@ -274,7 +316,7 @@ func runRun(opts *RunOptions) error {
return fmt.Errorf("could not create workflow dispatch event: %w", err)
}
if opts.IO.CanPrompt() {
if opts.IO.IsStdoutTTY() {
out := opts.IO.Out
cs := opts.IO.ColorScheme()
fmt.Fprintf(out, "%s Created workflow_dispatch event for %s at %s\n",
@ -314,21 +356,28 @@ func findInputs(yamlContent []byte) (map[string]WorkflowInput, error) {
// TODO this is pretty hideous
for _, node := range rootNode.Content[0].Content {
if onKeyNode != nil {
for _, node := range node.Content {
if dispatchKeyNode != nil {
for _, node := range node.Content {
if inputsKeyNode != nil {
inputsMapNode = node
break
}
if node.Value == "inputs" {
inputsKeyNode = node
if node.Kind == yaml.MappingNode {
for _, node := range node.Content {
if dispatchKeyNode != nil {
for _, node := range node.Content {
if inputsKeyNode != nil {
inputsMapNode = node
break
}
if node.Value == "inputs" {
inputsKeyNode = node
}
}
break
}
if node.Value == "workflow_dispatch" {
dispatchKeyNode = node
}
break
}
} else if node.Kind == yaml.ScalarNode {
if node.Value == "workflow_dispatch" {
dispatchKeyNode = node
break
}
}
break

View file

@ -4,8 +4,10 @@ import (
"bytes"
"encoding/base64"
"encoding/json"
"fmt"
"io/ioutil"
"net/http"
"os"
"testing"
"github.com/cli/cli/api"
@ -51,42 +53,38 @@ func TestNewCmdRun(t *testing.T) {
},
},
{
name: "extra args",
name: "both STDIN and input fields",
stdin: "some json",
cli: "workflow.yml -fhey=there",
errMsg: "only one of STDIN or -f/-F can be passed",
wantsErr: true,
},
{
name: "-f args",
tty: true,
cli: `workflow.yml -- --cool=nah --foo bar`,
cli: `workflow.yml -fhey=there -fname="dana scully"`,
wants: RunOptions{
InputArgs: []string{"--cool=nah", "--foo", "bar"},
Selector: "workflow.yml",
RawFields: []string{"hey=there", "name=dana scully"},
},
},
{
name: "both json on STDIN and json arg",
cli: `workflow.yml --json '{"cool":"yeah"}'`,
stdin: `{"cool":"yeah"}`,
wantsErr: true,
errMsg: "JSON can only be passed on one of STDIN or --json at a time",
},
{
name: "both json on STDIN and extra args",
cli: `workflow.yml -- --cool=nah`,
stdin: `{"cool":"yeah"}`,
errMsg: "only one of JSON or input arguments can be passed at a time",
wantsErr: true,
},
{
name: "both json arg and extra args",
tty: true,
cli: `workflow.yml --json '{"cool":"yeah"}' -- --cool=nah`,
errMsg: "only one of JSON or input arguments can be passed at a time",
wantsErr: true,
},
{
name: "json via argument",
cli: `workflow.yml --json '{"cool":"yeah"}'`,
name: "-F args",
tty: true,
cli: `workflow.yml -Fhey=there -Fname="dana scully" -Ffile=@cool.txt`,
wants: RunOptions{
JSON: `{"cool":"yeah"}`,
Selector: "workflow.yml",
Selector: "workflow.yml",
MagicFields: []string{"hey=there", "name=dana scully", "file=@cool.txt"},
},
},
{
name: "-F/-f arg mix",
tty: true,
cli: `workflow.yml -fhey=there -Fname="dana scully" -Ffile=@cool.txt`,
wants: RunOptions{
Selector: "workflow.yml",
RawFields: []string{"hey=there"},
MagicFields: []string{`name=dana scully`, "file=@cool.txt"},
},
},
{
@ -142,12 +140,86 @@ func TestNewCmdRun(t *testing.T) {
assert.Equal(t, tt.wants.Prompt, gotOpts.Prompt)
assert.Equal(t, tt.wants.JSON, gotOpts.JSON)
assert.Equal(t, tt.wants.Ref, gotOpts.Ref)
assert.ElementsMatch(t, tt.wants.InputArgs, gotOpts.InputArgs)
assert.ElementsMatch(t, tt.wants.RawFields, gotOpts.RawFields)
assert.ElementsMatch(t, tt.wants.MagicFields, gotOpts.MagicFields)
})
}
}
func Test_magicFieldValue(t *testing.T) {
f, err := ioutil.TempFile("", "gh-test")
if err != nil {
t.Fatal(err)
}
fmt.Fprint(f, "file contents")
f.Close()
t.Cleanup(func() { os.Remove(f.Name()) })
io, _, _, _ := iostreams.Test()
type args struct {
v string
opts RunOptions
}
tests := []struct {
name string
args args
want interface{}
wantErr bool
}{
{
name: "string",
args: args{v: "hello"},
want: "hello",
wantErr: false,
},
{
name: "file",
args: args{
v: "@" + f.Name(),
opts: RunOptions{IO: io},
},
want: "file contents",
wantErr: false,
},
{
name: "file error",
args: args{
v: "@",
opts: RunOptions{IO: io},
},
want: nil,
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := magicFieldValue(tt.args.v, tt.args.opts)
if (err != nil) != tt.wantErr {
t.Errorf("magicFieldValue() error = %v, wantErr %v", err, tt.wantErr)
return
}
if tt.wantErr {
return
}
assert.Equal(t, tt.want, got)
})
}
}
func TestRun(t *testing.T) {
minimalYAMLContent := []byte(`
name: minimal workflow
on: workflow_dispatch
jobs:
yell:
runs-on: ubuntu-latest
steps:
- name: do a yell
run: |
echo "AUUUGH!"
`)
encodedMinimalYAMLContent := base64.StdEncoding.EncodeToString(minimalYAMLContent)
yamlContent := []byte(`
name: a workflow
on:
@ -167,7 +239,7 @@ jobs:
run: |
echo "${{ github.event.inputs.greeting}}, ${{ github.events.inputs.name }}!"`)
encodedYamlContent := base64.StdEncoding.EncodeToString(yamlContent)
encodedYAMLContent := base64.StdEncoding.EncodeToString(yamlContent)
stubs := func(reg *httpmock.Registry) {
reg.Register(
@ -176,11 +248,6 @@ jobs:
Path: ".github/workflows/workflow.yml",
ID: 12345,
}))
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/contents/.github/workflows/workflow.yml"),
httpmock.JSONResponse(struct{ Content string }{
Content: encodedYamlContent,
}))
reg.Register(
httpmock.REST("POST", "repos/OWNER/REPO/actions/workflows/12345/dispatches"),
httpmock.StatusStringResponse(204, "cool"))
@ -209,11 +276,6 @@ jobs:
httpmock.JSONResponse(shared.Workflow{
Path: ".github/workflows/workflow.yml",
}))
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/contents/.github/workflows/workflow.yml"),
httpmock.JSONResponse(struct{ Content string }{
Content: encodedYamlContent,
}))
},
wantErr: true,
errOut: "could not parse provided JSON: unexpected end of JSON input",
@ -248,6 +310,22 @@ jobs:
},
httpStubs: stubs,
},
{
name: "nontty good input fields",
opts: &RunOptions{
Selector: "workflow.yml",
RawFields: []string{`name=scully`},
MagicFields: []string{`greeting=hey`},
},
wantBody: map[string]interface{}{
"inputs": map[string]interface{}{
"name": "scully",
"greeting": "hey",
},
"ref": "trunk",
},
httpStubs: stubs,
},
{
name: "respects ref",
tty: true,
@ -266,6 +344,7 @@ jobs:
wantOut: "✓ Created workflow_dispatch event for workflow.yml at good-branch\n\nTo see runs for this workflow, try: gh run list --workflow=workflow.yml\n",
},
{
// TODO this test is somewhat silly; it's more of a placeholder in case I decide to handle the API error more elegantly
name: "good JSON, missing required input",
tty: true,
opts: &RunOptions{
@ -277,76 +356,35 @@ jobs:
httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/workflow.yml"),
httpmock.JSONResponse(shared.Workflow{
Path: ".github/workflows/workflow.yml",
ID: 12345,
}))
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/contents/.github/workflows/workflow.yml"),
httpmock.JSONResponse(struct{ Content string }{
Content: encodedYamlContent,
}))
httpmock.REST("POST", "repos/OWNER/REPO/actions/workflows/12345/dispatches"),
httpmock.StatusStringResponse(422, "missing something"))
},
wantErr: true,
errOut: "missing required input 'name'",
errOut: "could not create workflow dispatch event: HTTP 422 (https://api.github.com/repos/OWNER/REPO/actions/workflows/12345/dispatches)",
},
{
name: "input arguments",
httpStubs: stubs,
tty: true,
// TODO this test is somewhat silly; it's more of a placeholder in case I decide to handle the API error more elegantly
name: "input fields, missing required",
opts: &RunOptions{
Selector: "workflow.yml",
InputArgs: []string{"--name", "scully"},
},
wantBody: map[string]interface{}{
"inputs": map[string]interface{}{
"name": "scully",
"greeting": "hi",
},
"ref": "trunk",
},
wantOut: "✓ Created workflow_dispatch event for workflow.yml at trunk\n\nTo see runs for this workflow, try: gh run list --workflow=workflow.yml\n",
},
{
name: "good JSON, missing required input",
tty: true,
opts: &RunOptions{
Selector: "workflow.yml",
InputArgs: []string{"--greeting=hey"},
RawFields: []string{`greeting="hello there"`},
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/workflow.yml"),
httpmock.JSONResponse(shared.Workflow{
Path: ".github/workflows/workflow.yml",
ID: 12345,
}))
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/contents/.github/workflows/workflow.yml"),
httpmock.JSONResponse(struct{ Content string }{
Content: encodedYamlContent,
}))
httpmock.REST("POST", "repos/OWNER/REPO/actions/workflows/12345/dispatches"),
httpmock.StatusStringResponse(422, "missing something"))
},
wantErr: true,
errOut: "missing required input 'name'",
},
{
name: "good JSON, missing required input",
tty: true,
opts: &RunOptions{
Selector: "workflow.yml",
InputArgs: []string{"--name=scully", "--bad=corrupt"},
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/workflow.yml"),
httpmock.JSONResponse(shared.Workflow{
Path: ".github/workflows/workflow.yml",
}))
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/contents/.github/workflows/workflow.yml"),
httpmock.JSONResponse(struct{ Content string }{
Content: encodedYamlContent,
}))
},
wantErr: true,
errOut: "could not parse input args: unknown flag: --bad",
errOut: "could not create workflow dispatch event: HTTP 422 (https://api.github.com/repos/OWNER/REPO/actions/workflows/12345/dispatches)",
},
{
name: "prompt, no workflows enabled",
@ -386,6 +424,43 @@ jobs:
wantErr: true,
errOut: "could not fetch workflows for OWNER/REPO: no workflows are enabled",
},
{
name: "prompt, minimal yaml",
tty: true,
opts: &RunOptions{
Prompt: true,
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"),
httpmock.JSONResponse(shared.WorkflowsPayload{
Workflows: []shared.Workflow{
{
Name: "minimal workflow",
ID: 1,
State: shared.Active,
Path: ".github/workflows/minimal.yml",
},
},
}))
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/contents/.github/workflows/minimal.yml"),
httpmock.JSONResponse(struct{ Content string }{
Content: encodedMinimalYAMLContent,
}))
reg.Register(
httpmock.REST("POST", "repos/OWNER/REPO/actions/workflows/1/dispatches"),
httpmock.StatusStringResponse(204, "cool"))
},
askStubs: func(as *prompt.AskStubber) {
as.StubOne(0)
},
wantBody: map[string]interface{}{
"inputs": map[string]interface{}{},
"ref": "trunk",
},
wantOut: "✓ Created workflow_dispatch event for minimal.yml at trunk\n\nTo see runs for this workflow, try: gh run list --workflow=minimal.yml\n",
},
{
name: "prompt",
tty: true,
@ -408,7 +483,7 @@ jobs:
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/contents/.github/workflows/workflow.yml"),
httpmock.JSONResponse(struct{ Content string }{
Content: encodedYamlContent,
Content: encodedYAMLContent,
}))
reg.Register(
httpmock.REST("POST", "repos/OWNER/REPO/actions/workflows/12345/dispatches"),