feat!: Add support for multi-select Custom Properties

[arymoraes]

Fixes#3198.

BREAKING CHANGE: CustomPropertyValue.Value is changed from *string to interface{} to support string and []string values.

So the problem here is that the GitHub custom properties value can come in either as a string (for all non multi_select properties), or as a []string in a multi_select property.

The issue is happening because we were trying to unmarshal it but always expecting a string for the Value.

My solution was to make value an interface{} type that implements a UnmarshalJSON to handle both scenarios. While this works, we end up losing the auto generated GetValue() accessor. Another thing I was unsure is that I saw on other places and on the tests for this that we use a pointer to the value, but I changed it to the value itself on the Unmarshal call since we lose the accessor, not sure if this is correct.

Another solution I played with was using json.RawMessage, I could not make it fully work (I can give it another try), but at least we did lose the auto generated GetValue accessor there, would this be a more adequate solution?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (2b8c7fa) to head (ba66916).
Report is 76 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@## master #3200 +/- ##
==========================================
- Coverage 97.72% 92.92% -4.80% 
==========================================
Files 153 171 +18 Lines 13390 11582 -1808 ==========================================
- Hits 13085 10763 -2322 - Misses 215 726 +511 - Partials 90 93 +3 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gmlewis]

Yes, if I remember correctly, we handle situations like this in various ways:

• with interface{} like you did (e.g. copilot )
• with json.RawMessage like you mention (e.g. scim , repos_deployments , repos_rules , etc.)
• with multiple structs (not finding any good examples at the moment)

So I'm fine with this method. You could additionally define two new structs CustomPropertyValueWithString and CustomPropertyValueWithStringSlice that have the respective types for Value and then two helper functions could each return one of the new types with a bool (indicating if the returned pointer is actually that type or not) if you want... but I'm fine if you want to just run with the current solution too.

[arymoraes]

Yes, if I remember correctly, we handle situations like this in various ways:

• with interface{} like you did (e.g. copilot )
• with json.RawMessage like you mention (e.g. scim , repos_deployments , repos_rules , etc.)
• with multiple structs (not finding any good examples at the moment)

So I'm fine with this method. You could additionally define two new structs CustomPropertyValueWithString and CustomPropertyValueWithStringSlice that have the respective types for Value and then two helper functions could each return one of the new types with a bool (indicating if the returned pointer is actually that type or not) if you want... but I'm fine if you want to just run with the current solution too.

That makes sense, thank you for the explanation and the breakdown. I'm fine with the current solution as is if we want to go with that, at some point I did something similar to having two new structs but ended up deciding to move back to trying the interface{} approach.

[gmlewis]

Thank you, @arymoraes!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

[gmlewis]

Thank you, @valbeat !
Merging.

[darend]

Do boolean custom properties come through as a string as well?

https://docs.github.com/en/organizations/managing-organization-settings/managing-custom-properties-for-repositories-in-your-organization#adding-custom-properties

[arymoraes]

Yes, they come as string from the GitHub API:

 {
"property_name": "boolean_prop",
"value": "true"
},