-
Notifications
You must be signed in to change notification settings - Fork 7
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
[BUG]: PATCH request for repos/OWNER/REPO/properties/values broken #90
Comments
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
I appreciate this super thoughtful bug report, and thank you for working to bring the new SDK into the Terraform provider. That's great! I've reproduced what you've done here on a dummy repo (kfcampbell-terraform-provider/tf-acc-test-destroy-trfiz), and I agree there's something wonky going on in the generated code. I'm able to add custom properties successfully via the UI, and via a CURL request like the following:
When using the SDK, I'm using the following code (after creating the client as we do in the example token CLI): customPropertyValue := models.NewCustomPropertyValue()
propertyName := "environment"
propertyValue := "test-from-go-sdk"
customPropertyValue.SetPropertyName(&propertyName)
value := models.NewCustomPropertyValue_CustomPropertyValue_value()
value.SetString(&propertyValue)
customPropertyValue.SetValue(value)
patchRequestBody := repos.NewItemItemPropertiesValuesPatchRequestBody()
patchRequestBody.SetProperties([]models.CustomPropertyValueable{customPropertyValue})
err = client.Repos().ByOwnerId("kfcampbell-terraform-provider").ByRepoId("tf-acc-test-destroy-trfiz").
Properties().Values().Patch(context.Background(), patchRequestBody, nil)
if err != nil {
log.Fatalf("error setting custom properties: %v", err)
} This fails with a 400 Bad Request error, and I see the payload going out as:
When I attempt a curl request with that as the payload (as I'm somewhat suspicious of the backslashes), I get a 422, not a 400. I think this is an error with Kiota, our generation engine, so I've filed microsoft/kiota#4995 and will do some investigation with them. |
Thanks! I'll take a stab after work today or tomorrow |
@kfcampbell setting non-
This might warrant its own issue though 😄 I'll also note that the Organization Ruleset API seems to handle the fact that a property value can be either a string or a list of strings in the way I proposed here, i.e., by just treating it as a list of strings at all times. It's pretty deeply nested, but Org Rulesets can target repos by custom properties, and this is where this occurs. |
What happened?
I'm playing around with this sdk as a means of contributing to integrations/terraform-provider-github#1956, and as such my focus is on the endpoints related to repository custom properties.
The issue I'm facing occurs when calling octokitClient.Repos().ByOwnerId(owner).ByRepoId(repoName).Properties().Values().Patch(), which is mapped to the repos/OWNER/REPO/properties/values
PATCH
endpoint. I'll list the code I'm using further down in the message.When running the code below I receive the following error:
error status code received from the API
I've ran the debugger and dug deeper into the call chain, and found that the error code is
400
, but haven't been able to find an error message. Digging down all the way to thenet/http
layer and inspecting the request body being sent, it looks like this:"{\"properties\":[{\"property_name\":\"text\",\"value\":\"test\"}]}"
Which isn't too dissimilar to what the API expects (example from the docs):
'{"properties":[{"property_name":"environment","value":"production"},{"property_name":"service","value":"web"},{"property_name":"team","value":"octocat"}]}'
After playing around with
curl
a bit, I've managed to receive a400
error by using malformed JSON in the body (see example below). My guess is that this SDK somewhere along the line parses the custom property incorrectly, or that I'm passing theproperty_name
andvalue
incorrectly to it.I've also explored the following:
value
attribute of anCustomPropertyValue
should implement theCustomPropertyValue_CustomPropertyValue_valueable
interface, which only have the two methodsGetString()
andSetString
, which implies that the value of a custom property can only be a string. But it can be either a string or a list of string (multi_select
custom property type). Due to this this SDK can only work with the other property types currently (except for the stuff mentioned above 😄). Thecurl
call below is valid, but there is no way of replicating it with this SDK from what I can tellmodels.CustomPropertyValue
andrepos.ItemItemPropertiesValuesPatchRequestBody
supports a function calledSetAdditionalData()
. I've tried playing around with those two, but they seem to mess up the json body that is being sent to the GitHub API by adding key value pairs to it (the two"test": "text"
below):Versions
I've used the latest version of the provider at the time of writing this bug report (
go get github.com/octokit/go-sdk@a74a3a2a13654bd84794fd91a6a0fbc96f883722
, wherea74a3a2a13654bd84794fd91a6a0fbc96f883722
is a commit to this repo). Mygo.mod
file looks like so:Code of Conduct
The text was updated successfully, but these errors were encountered: