mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-12-07 14:09:47 +00:00
fix: accept true as input in workflow_dispatch (#10089)
Previously, when triggering workflows with `workflow_dispatch`, Forgejo only interpreted `on` as boolean `true`. Everything else, including `true`, was treated as `false`. This behaviour does not match the [Forgejo documentation](https://forgejo.org/docs/v13.0/user/actions/reference/#onworkflow_dispatch) that states that `true` and `false` are permitted values. It is also outside the [YAML 1.2 specification of booleans](https://yaml.org/spec/1.2.2/#10212-boolean) that only permits `true` and `false`. After this change, only `true` and `false` have the desired effect. `on` (converted to `true`) is kept for compatibility reasons to give people time to upgrade. This problem only affected users of the Forgejo API, because the UI sent the expected values. Resolves forgejo/forgejo#10070 by fixing the documentation mismatch. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10089 Reviewed-by: klausfyhn <klausfyhn@noreply.codeberg.org> Reviewed-by: Gusted <gusted@noreply.codeberg.org> Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch> Co-committed-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
This commit is contained in:
Andreas Ahlenstorf
Gusted
parent
35fa852406
commit
8fea4c5829
3 changed files with 161 additions and 21 deletions
|
|
@ -8,7 +8,6 @@ import (
|
|||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"strconv"
|
||||
|
||||
actions_model "forgejo.org/models/actions"
|
||||
"forgejo.org/models/perm"
|
||||
|
|
@ -50,6 +49,28 @@ type Workflow struct {
|
|||
|
||||
type InputValueGetter func(key string) string
|
||||
|
||||
func resolveDispatchInput(key, value string, input act_model.WorkflowDispatchInput) (string, error) {
|
||||
if len(value) == 0 {
|
||||
value = input.Default
|
||||
if len(value) == 0 {
|
||||
if input.Required {
|
||||
name := input.Description
|
||||
if len(name) == 0 {
|
||||
name = key
|
||||
}
|
||||
return "", InputRequiredErr{Name: name}
|
||||
}
|
||||
}
|
||||
} else if input.Type == "boolean" {
|
||||
// Temporary compatibility shim for people that upgrade to Forgejo 14. Can be removed with Forgejo 15.
|
||||
if value == "on" {
|
||||
value = "true"
|
||||
}
|
||||
}
|
||||
|
||||
return value, nil
|
||||
}
|
||||
|
||||
func (entry *Workflow) Dispatch(ctx context.Context, inputGetter InputValueGetter, repo *repo_model.Repository, doer *user.User) (r *actions_model.ActionRun, j []string, err error) {
|
||||
content, err := actions.GetContentFromEntry(entry.GitEntry)
|
||||
if err != nil {
|
||||
|
|
@ -72,25 +93,12 @@ func (entry *Workflow) Dispatch(ctx context.Context, inputGetter InputValueGette
|
|||
inputsAny := make(map[string]any)
|
||||
if workflowDispatch := wf.WorkflowDispatchConfig(); workflowDispatch != nil {
|
||||
for key, input := range workflowDispatch.Inputs {
|
||||
val := inputGetter(key)
|
||||
if len(val) == 0 {
|
||||
val = input.Default
|
||||
if len(val) == 0 {
|
||||
if input.Required {
|
||||
name := input.Description
|
||||
if len(name) == 0 {
|
||||
name = key
|
||||
}
|
||||
return nil, nil, InputRequiredErr{Name: name}
|
||||
}
|
||||
continue
|
||||
}
|
||||
} else if input.Type == "boolean" {
|
||||
// Since "boolean" inputs are rendered as a checkbox in html, the value inside the form is "on"
|
||||
val = strconv.FormatBool(val == "on")
|
||||
value, err := resolveDispatchInput(key, inputGetter(key), input)
|
||||
if err != nil {
|
||||
return nil, nil, err
|
||||
}
|
||||
inputs[key] = val
|
||||
inputsAny[key] = val
|
||||
inputs[key] = value
|
||||
inputsAny[key] = value
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -121,3 +121,135 @@ func TestConfigureActionRunConcurrency(t *testing.T) {
|
|||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveDispatchInputAcceptsValidInput(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
key string
|
||||
value string
|
||||
input act_model.WorkflowDispatchInput
|
||||
expected string
|
||||
}{
|
||||
{
|
||||
name: "on_converted_to_true",
|
||||
key: "my_boolean",
|
||||
value: "on",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a boolean", Required: false, Type: "boolean", Options: []string{}},
|
||||
expected: "true",
|
||||
},
|
||||
// It might make sense to validate booleans in the future and then turn it into an error.
|
||||
{
|
||||
name: "ON_stays_ON",
|
||||
key: "my_boolean",
|
||||
value: "ON",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a boolean", Required: false, Type: "boolean", Options: []string{}},
|
||||
expected: "ON",
|
||||
},
|
||||
{
|
||||
name: "true_stays_true",
|
||||
key: "my_boolean",
|
||||
value: "true",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a boolean", Required: false, Type: "boolean", Options: []string{}},
|
||||
expected: "true",
|
||||
},
|
||||
{
|
||||
name: "false_stays_false",
|
||||
key: "my_boolean",
|
||||
value: "false",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a boolean", Required: false, Type: "boolean", Options: []string{}},
|
||||
expected: "false",
|
||||
},
|
||||
{
|
||||
name: "empty_results_in_default_value_true",
|
||||
key: "my_boolean",
|
||||
value: "",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a boolean", Required: false, Default: "true", Type: "boolean", Options: []string{}},
|
||||
expected: "true",
|
||||
},
|
||||
{
|
||||
name: "empty_results_in_default_value_false",
|
||||
key: "my_boolean",
|
||||
value: "",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a boolean", Required: false, Default: "false", Type: "boolean", Options: []string{}},
|
||||
expected: "false",
|
||||
},
|
||||
{
|
||||
name: "string_results_in_input",
|
||||
key: "my_string",
|
||||
value: "hello",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a string", Required: false, Type: "string", Options: []string{}},
|
||||
expected: "hello",
|
||||
},
|
||||
{
|
||||
name: "string_option_results_in_input",
|
||||
value: "a",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a string", Required: false, Type: "string", Options: []string{"a", "b"}},
|
||||
expected: "a",
|
||||
},
|
||||
// Test ensures that the old behaviour (ignoring option mismatch) is retained. It might
|
||||
// make sense to turn it into an error in the future.
|
||||
{
|
||||
name: "invalid_string_option_results_in_input",
|
||||
key: "option",
|
||||
value: "c",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a string", Required: false, Type: "string", Options: []string{"a", "b"}},
|
||||
expected: "c",
|
||||
},
|
||||
{
|
||||
name: "number_results_in_input",
|
||||
key: "my_number",
|
||||
value: "123",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a string", Required: false, Type: "number", Options: []string{}},
|
||||
expected: "123",
|
||||
},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
actual, _ := resolveDispatchInput(tc.key, tc.value, tc.input)
|
||||
assert.Equal(t, tc.expected, actual)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveDispatchInputRejectsInvalidInput(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
key string
|
||||
value string
|
||||
input act_model.WorkflowDispatchInput
|
||||
expected error
|
||||
}{
|
||||
{
|
||||
name: "missing_required_boolean",
|
||||
key: "missing_boolean",
|
||||
value: "",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a boolean", Required: true, Type: "boolean", Options: []string{}},
|
||||
expected: InputRequiredErr{Name: "a boolean"},
|
||||
},
|
||||
{
|
||||
name: "missing_required_boolean_without_description",
|
||||
key: "missing_boolean",
|
||||
value: "",
|
||||
input: act_model.WorkflowDispatchInput{Required: true, Type: "boolean", Options: []string{}},
|
||||
expected: InputRequiredErr{Name: "missing_boolean"},
|
||||
},
|
||||
{
|
||||
name: "missing_required_string",
|
||||
key: "missing_string",
|
||||
value: "",
|
||||
input: act_model.WorkflowDispatchInput{Description: "a string", Required: true, Type: "string", Options: []string{}},
|
||||
expected: InputRequiredErr{Name: "a string"},
|
||||
},
|
||||
{
|
||||
name: "missing_required_string_without_description",
|
||||
key: "missing_string",
|
||||
value: "",
|
||||
input: act_model.WorkflowDispatchInput{Required: true, Type: "string", Options: []string{}},
|
||||
expected: InputRequiredErr{Name: "missing_string"},
|
||||
},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
_, err := resolveDispatchInput(tc.key, tc.value, tc.input)
|
||||
assert.Equal(t, tc.expected, err)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -34,8 +34,8 @@
|
|||
<div class="ui checkbox">
|
||||
<label><strong>{{if $val.Description}}{{$val.Description}}{{else}}{{$key}}{{end}}</strong></label>
|
||||
{{/* These two inputs need to stay in exactly this order (checkbox first, hidden second) or boolean fields won't work correctly! */}}
|
||||
<input type="checkbox" name="inputs[{{$key}}]" value="on" {{if eq $val.Default "true"}}checked{{end}}>
|
||||
<input type="hidden" name="inputs[{{$key}}]" value="off" autocomplete="off">
|
||||
<input type="checkbox" name="inputs[{{$key}}]" value="true" {{if eq $val.Default "true"}}checked{{end}}>
|
||||
<input type="hidden" name="inputs[{{$key}}]" value="false" autocomplete="off">
|
||||
</div>
|
||||
{{else}}
|
||||
<label>{{if $val.Description}}{{$val.Description}}{{else}}{{$key}}{{end}}</label>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue