feat: modified choice input to take option list opposed to blank text

[adarshjhaa100]

Fixes #8017

[williammartin]

Hey @adarshjhaa100 thank you for opening this PR. It definitely seems like a nice addition to the run workflow. I've left a couple of comments on things that I want to see changed. I also want to ensure we add tests for this because code coverage says none of this new code is covered:

I already wrote some tests while reviewing this but I'd like to give you the learning opportunity first. If you'd prefer that I simply add the tests myself then I can do that but I'd also ask for you to review that so you can learn.

Cheers and thanks for your contribution!

[williammartin]

I understand what you're going for here, that the description should provide a better prompt than the Name but unfortunately, I don't think it's a required field in the YAML so you can end up with this:

It would also be better to be consistent with the other inputs that show their name.

[williammartin]

We should probably provide input.Default as the second argument here?

[williammartin]

Suggested change

	return nil, fmt.Errorf("workflow input %s of type choice should has non zero options", name)
	return nil, fmt.Errorf("workflow input %q is of type choice, but has no options", name)

[williammartin]

This isn't correct. What this is saying is "if there is an error prompting, then break out of this loop" where breaking out of the loop results in the submission of the providedInputs. This is different than the break below which only breaks out of the required for {}. The error return actually happens on line 243.

I don't really like this pattern because it causes easy confusion as we've seen here. I think it's almost always better to return an error immediately. Please change this to:

if err != nil {
   return nil, err
}

[williammartin]

An error can occur for example, if I hit ctrl+C while being prompted. I would expect that to exit the CLI but in this case it actually ends up submitting the workflow!

[adarshjhaa100]

Hello @williammartin, Thanks for the review! I'll make the required changes asked for. They do seem reasonable based on the functionality and standards followed across the Codebase.

Also, I'd really like to add a test case for this PR so that I can really learn something new (it's my first time working on a Go project).

I do have a small question though. The test cases should be written similar this right? And, should I create a new function or add the tests to this TestRun or any of the existing ones.

cli/pkg/cmd/workflow/run/run_test.go

Lines 319 to 331 in 08bc4cb

	func TestRun(t *testing.T) {
	noInputsYAMLContent := []byte(`
	name: minimal workflow
	on: workflow_dispatch
	jobs:
	yell:
	runs-on: ubuntu-latest
	steps:
	- name: do a yell
	run: |
	echo "AUUUGH!"
	`)
	encodedNoInputsYAMLContent := base64.StdEncoding.EncodeToString(noInputsYAMLContent)

[williammartin]

Most likely I would add the happy path test into this content:

cli/pkg/cmd/workflow/run/run_test.go

Lines 332 to 349 in 08bc4cb

	yamlContent := []byte(`
	name: a workflow
	on:
	workflow_dispatch:
	inputs:
	greeting:
	default: hi
	description: a greeting
	name:
	required: true
	description: a name
	jobs:
	greet:
	runs-on: ubuntu-latest
	steps:
	- name: perform the greet
	run: |
	echo "${{ github.event.inputs.greeting}}, ${{ github.events.inputs.name }}!"`)

and update the prompt stubs and expected output here:

cli/pkg/cmd/workflow/run/run_test.go

Lines 623 to 641 in 08bc4cb

	promptStubs: func(pm *prompter.MockPrompter) {
	pm.RegisterSelect("Select a workflow", []string{"a workflow (workflow.yml)"}, func(_, _ string, opts []string) (int, error) {
	return 0, nil
	})
	pm.RegisterInput("greeting", func(_, _ string) (string, error) {
	return "hi", nil
	})
	pm.RegisterInput("name (required)", func(_, _ string) (string, error) {
	return "scully", nil
	})
	},
	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",

I would probably create a new subtest with an inline workflow to test when there are no options for the choice.

[adarshjhaa100]

@williammartin Pushed the review changes and added subtests for choice inputs:

• Happy path test - with two inputs : required and default
• Missing choice input options

Do let me know in case any modifications are required.

[williammartin]

LGTM thank you for your contribution to the CLI!