cli/cli

`gh api -F <key=val>` not accepting `int` or `boolean` on ruleset creation

AlexVermette-Eaton opened this issue · 12 comments

AlexVermette-Eaton commented

Describe the bug

Version : gh version 2.48.0

Using the gh api command to create ruleset, it is impossible to pass anything other than a string as a "key=value" pair. I tried using -F flag but it does not work either.

Here's an example:

    gh api -X POST \
    -H "Accept: application/vnd.github+json"\
    -H "X-GitHub-Api-Version: 2022-11-28"\
     repos/$owner/$repo/rulesets\
    -f "name=Test"\
    -f "target=branch"\
    -f "enforcement=active"\
    -f "rules[][type]=pull_request"\
    -F "rules[][parameters][required_approving_review_count]=1"

I get the following error:

{
  "message": "Invalid request.\n\nInvalid property /rules/0: data matches no possible input. See `documentation_url`.",
  "documentation_url": "https://docs.github.com/rest/repos/rules#create-a-repository-ruleset"
}

I get the same behavior when i try with a property that should have a boolean value. For example:

    gh api -X POST \
    -H "Accept: application/vnd.github+json"\
    -H "X-GitHub-Api-Version: 2022-11-28"\
     repos/$owner/$repo/rulesets\
    -f "name=Test"\
    -f "target=branch"\
    -f "enforcement=active"\
    -f "rules[][type]=pull_request"\
    -F "rules[][parameters][dismiss_stale_reviews_on_push]=true"
williammartin commented

I see the same behaviour on v2.48.0 and v2.47.0 but I wonder if this is a platform bug.

Looking at your examples using the --verbose flag, the body looks correct to me:

{
  "enforcement": "active",
  "name": "Test",
  "rules": [
    {
      "parameters": {
        "required_approving_review_count": 1
      },
      "type": "pull_request"
    }
  ],
  "target": "branch"
}
{
  "enforcement": "active",
  "name": "Test",
  "rules": [
    {
      "parameters": {
        "dismiss_stale_reviews_on_push": true
      },
      "type": "pull_request"
    }
  ],
  "target": "branch"
}

Is there anything that looks wrong in these request to you? I must admit I'm not an expert on the ruleset API. Also, the example gh snippet in those docs is definitely broken in some ways.

andyfeller commented

@AlexVermette-Eaton : Just to share how I was testing #8762 by creating rule:

QUERY='
mutation($input:CreateRepositoryRulesetInput!){
  createRepositoryRuleset(input:$input){
    ruleset{
      id
      name
      createdAt
    }
  }
}
'

GH_DEBUG=api ./bin/gh api graphql -f query="$QUERY" \
	-F 'input[name]=test repo rule name3' \
	-F 'input[enforcement]=DISABLED' \
	-F 'input[conditions][refName][include][]=~DEFAULT_BRANCH' \
	-F 'input[conditions][refName][exclude][]=refs/heads/ignore-*' \
	-F 'input[sourceId]=R_kgDOIVGotg' \
	-F 'input[rules][][type]=CREATION'

Let me be clear, this took a bit of reversing by manually creating a rule and playing with it.

  • ❤️1
AlexVermette-Eaton commented

@williammartin

Is there anything that looks wrong in these request to you?

The requests look good to me. This is exactly what i would expect. To get the latest structure of the ruleset object i used the export tool of rulesets available in the GitHub UI and looked at the json file it gave.

Also, the example gh snippet in those docs is definitely broken in some ways.

It definitely is broken. I should have probably reported this too but here's what I found that is broken:

  • The ref_name[include] is not valid, what needs to be used is something like conditions[ref_name][include]
  • same goes for ref_name[exclude]

I would assume bypass_actors[][actor_id]=234 is also broken for the same reason as my 2 examples above but I have not tested this particular key in my use cases.

AlexVermette-Eaton commented

If anyone has a work around I would gladly take it, as this is blocking for what
I am trying to achieve.

williammartin commented

I left a note for the team that owns this API endpoint on Saturday but given that it was the weekend I wouldn't expect a reply. In terms of your original comment, the way to have the most control is to craft your own JSON body and go from there:

{
  "query": "mutation($input:CreateRepositoryRulesetInput!){\n  createRepositoryRuleset(input:$input){\n    ruleset{\n      id\n      name\n      createdAt\n    }\n  }\n}",
  "variables": {
    "input": {
      "enforcement": "ACTIVE",
      "name": "Test",
      "conditions": {},
      "rules": [
        {
          "parameters": {
            "pullRequest": {
              "dismissStaleReviewsOnPush": false,
              "requireCodeOwnerReview": false,
              "requireLastPushApproval": false,
              "requiredApprovingReviewCount": 1,
              "requiredReviewThreadResolution": false
            }
          },
          "type": "PULL_REQUEST"
        }
      ],
      "sourceId": "<REPOSITORY_GQL_ID>",
      "target": "BRANCH"
    }
  }
}
gh api graphql --verbose --input <FILE_CONTAINING_JSON_ABOVE>

You can get the <REPOSITORY_GQL_ID> via:

gh repo list <OWNER> --json "id,name"

I appreciate this is not particularly pleasant. I will get back to you when we hear back from the team that own the ruleset REST API.

  • ❤️1
williammartin commented

Looks like the issue is that for the parameters of a rule, all fields are required as in the docs:

Image

What this means is that you need a body like so:

{
  "enforcement": "active",
  "name": "Test",
  "rules": [
    {
      "parameters": {
        "dismiss_stale_reviews_on_push": false,
        "require_code_owner_review": false,
        "require_last_push_approval": false,
        "required_approving_review_count": 1,
        "required_review_thread_resolution": false
      },
      "type": "pull_request"
    }
  ],
  "target": "branch"
}

Could be used like so:

gh api --verbose -X POST \
--input <FILE_CONTAINING_JSON_ABOVE> \
repos/{owner}/{repo}/rulesets

The -F flags for fields are mostly suited for simple cases and can get janky very quickly with the syntax that was chosen. I'm not sure there's a way to represent this JSON structure via -F, so you may end up having to do some preprocessing to produce the JSON yourself (it can be passed on stdin with --input -. Have asked @andyfeller to see if he can massage -F because he knows things 🤓 . Does it matter to you whether -F works for this case?

andyfeller commented

Limitation of #8761 involving nested objects within arrays

@williammartin : I believe we're experiencing a limitation behind parseFields design as it unclear when to create new nested objects over multiple fields within an array over multiple field flags.

Explanation

The following explanation resolves around the following 2 blocks of code:

cli/pkg/cmd/api/fields.go

Lines 63 to 84 in 8181c62

destMap := params
isArray := false
var subkey string
for _, k := range keystack {
if k == "" {
isArray = true
continue
}
if subkey != "" {
var err error
if isArray {
destMap, err = addParamsSlice(destMap, subkey, k)
isArray = false
} else {
destMap, err = addParamsMap(destMap, subkey)
}
if err != nil {
return err
}
}
subkey = k

cli/pkg/cmd/api/fields.go

Lines 135 to 150 in 8181c62

func addParamsSlice(m map[string]interface{}, prevkey, newkey string) (map[string]interface{}, error) {
if v, exists := m[prevkey]; exists {
if existSlice, ok := v.([]interface{}); ok {
if len(existSlice) > 0 {
lastItem := existSlice[len(existSlice)-1]
if lastMap, ok := lastItem.(map[string]interface{}); ok {
if _, keyExists := lastMap[newkey]; !keyExists {
return lastMap, nil
} else if reflect.TypeOf(lastMap[newkey]).Kind() == reflect.Slice {
return lastMap, nil
}
}
}
newMap := make(map[string]interface{})
m[prevkey] = append(existSlice, newMap)
return newMap, nil

Take the following example:

gh api -X POST "repos/{owner}/{repo}/rulesets" \
    -H "Accept: application/vnd.github+json" \
    -H "X-GitHub-Api-Version: 2022-11-28" \
    -f "name=Test" \
    -f "target=branch" \
    -f "enforcement=active" \
    -F "rules[][type]=pull_request" \
    -F "rules[][parameters][dismiss_stale_reviews_on_push]=false" \
    -F "rules[][parameters][require_code_owner_review]=false" \
    -F "rules[][parameters][require_last_push_approval]=false" \
    -F "rules[][parameters][required_approving_review_count]=1" \
    -F "rules[][parameters][required_review_thread_resolution]=false"

The problem occurs because parseFields and related logic doesn't know when to start a new object within an array. It parses field keys one step at a time without taking later keys into account before deciding if this is setting an existing object or a new object.

Assuming the following state before rules field is being parsed:

{
    "name": "Test",
    "target": "branch",
    "enforcement": "active",
}

parseFields generates a stack of keys to set field values while detecting when an object is nested in an array:

  1. -F "rules[][type]=pull_request"

    parseFields sees rules as the previous key and type as the new key and is aware it is in an array but not how deep the array is. Because test doesn't exist within the map, addParamsSlice creates a new map and appends it to the array before being returned to set the value resulting in:

    {
    "name": "Test",
    "target": "branch",
    "enforcement": "active",
    "rules": [
        {
            "type": "pull_request"
        }
    ]
}

  2. -F "rules[][parameters][dismiss_stale_reviews_on_push]=false"

    Again, parseFields sees rules as the previous key and parameters as the new key and is aware it is in an array. Because parameters doesn't exist within the map, addParamsSlice creates a new map and appends it to the array before being returned to set the value resulting in:

    {
    "name": "Test",
    "target": "branch",
    "enforcement": "active",
    "rules": [
        {
            "type": "pull_request",
            "parameters": {
                "dismiss_stale_reviews_on_push": false
            }
        }
    ]
}

  3. -F "rules[][parameters][require_code_owner_review]=false"

    This is where we get into the problem: parseFields sees rules and parameters just like before. addParamsSlice sees parameters DOES exist however it doesn't fall into any existing use case to be reused as it doesn't have enough information to know we will set a totally different field than the last time.

    {
    "name": "Test",
    "target": "branch",
    "enforcement": "active",
    "rules": [
        {
            "type": "pull_request",
            "parameters": {
                "dismiss_stale_reviews_on_push": false
            }
        },
        {
            "parameters": {
                "require_code_owner_review": false
            }
        }
    ]
}

This feels like a problem with the field set design as users cannot set a whole object in a single flag.

Test for debugging

Locally, I created the following test within pkg/cmd/api/fields_test.go:

func Test_parseFields_nested2(t *testing.T) {
	ios, stdin, _, _ := iostreams.Test()
	fmt.Fprint(stdin, "pasted contents")

	opts := ApiOptions{
		IO: ios,
		RawFields: []string{
			"name=Test",
			"target=branch",
			"enforcement=active",
			"rules[][type]=pull_request",
		},
		MagicFields: []string{
			"rules[][parameters][dismiss_stale_reviews_on_push]=false",
			"rules[][parameters][require_code_owner_review]=false",
			"rules[][parameters][require_last_push_approval]=false",
			"rules[][parameters][required_approving_review_count]=1",
			"rules[][parameters][required_review_thread_resolution]=false",
		},
	}

	params, err := parseFields(&opts)
	if err != nil {
		t.Fatalf("parseFields error: %v", err)
	}

	jsonData, err := json.MarshalIndent(params, "", "\t")
	if err != nil {
		t.Fatal(err)
	}

	assert.Equal(t, strings.TrimSuffix(heredoc.Doc(`{}`), "\n"), string(jsonData))
}
AlexVermette-Eaton commented

@williammartin

The -F flags for fields are mostly suited for simple cases and can get janky very quickly with the syntax that was chosen. I'm not sure there's a way to represent this JSON structure via -F, so you may end up having to do some preprocessing to produce the JSON yourself (it can be passed on stdin with --input -. Have asked @andyfeller to see if he can massage -F because he knows things 🤓 . Does it matter to you whether -F works for this case?

Your suggested approach is answering all my needs. I don't specifically needed the -F flag to work, I just needed a way to pass the whole ruleset data to the creation API and what you suggested is 100% doing the job for me; I just tested it. You can either close this issue or leave it open if you think this still needs to be fixed. I would suggest maybe reworking the API documentation before closing this.

  • ❤️1