feat!: Widen CustomProperties type to map[string]interface{} to align with GitHub API #3230

spalberg · 2024-08-10T16:19:33Z

Fixes: #3229.

BREAKING CHANGE: PushEventRepository.CustomProperties is changed from map[string]string to map[string]interface{}.

adds tests for json unmarshalling of projects

google-cla · 2024-08-10T16:19:37Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

gmlewis

Thank you, @spalberg !
Just one comment, then we should be able to merge after a second LGTM+Approval from any other contributor to this repo.

gmlewis · 2024-08-10T16:54:50Z

github/repos_test.go

+
+	t.Run("go-github repo (sanity check)", func(t *testing.T) {
+		// regenerate using github cli: gh api repos/google/go-github
+		data := []byte(`{"id":10270722,"node_id":"MDEwOlJlcG9zaXRvcnkxMDI3MDcyMg==","name":"go-github","full_name":"google/go-github","private":false,"owner":{"login":"google","id":1342004,"node_id":"MDEyOk9yZ2FuaXphdGlvbjEzNDIwMDQ=","avatar_url":"https://avatars.githubusercontent.com/u/1342004?v=4","gravatar_id":"","url":"https://api.github.com/users/google","html_url":"https://github.com/google","followers_url":"https://api.github.com/users/google/followers","following_url":"https://api.github.com/users/google/following{/other_user}","gists_url":"https://api.github.com/users/google/gists{/gist_id}","starred_url":"https://api.github.com/users/google/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/google/subscriptions","organizations_url":"https://api.github.com/users/google/orgs","repos_url":"https://api.github.com/users/google/repos","events_url":"https://api.github.com/users/google/events{/privacy}","received_events_url":"https://api.github.com/users/google/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/google/go-github","description":"Go library for accessing the GitHub v3 API","fork":false,"url":"https://api.github.com/repos/google/go-github","forks_url":"https://api.github.com/repos/google/go-github/forks","keys_url":"https://api.github.com/repos/google/go-github/keys{/key_id}","collaborators_url":"https://api.github.com/repos/google/go-github/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/google/go-github/teams","hooks_url":"https://api.github.com/repos/google/go-github/hooks","issue_events_url":"https://api.github.com/repos/google/go-github/issues/events{/number}","events_url":"https://api.github.com/repos/google/go-github/events","assignees_url":"https://api.github.com/repos/google/go-github/assignees{/user}","branches_url":"https://api.github.com/repos/google/go-github/branches{/branch}","tags_url":"https://api.github.com/repos/google/go-github/tags","blobs_url":"https://api.github.com/repos/google/go-github/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/google/go-github/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/google/go-github/git/refs{/sha}","trees_url":"https://api.github.com/repos/google/go-github/git/trees{/sha}","statuses_url":"https://api.github.com/repos/google/go-github/statuses/{sha}","languages_url":"https://api.github.com/repos/google/go-github/languages","stargazers_url":"https://api.github.com/repos/google/go-github/stargazers","contributors_url":"https://api.github.com/repos/google/go-github/contributors","subscribers_url":"https://api.github.com/repos/google/go-github/subscribers","subscription_url":"https://api.github.com/repos/google/go-github/subscription","commits_url":"https://api.github.com/repos/google/go-github/commits{/sha}","git_commits_url":"https://api.github.com/repos/google/go-github/git/commits{/sha}","comments_url":"https://api.github.com/repos/google/go-github/comments{/number}","issue_comment_url":"https://api.github.com/repos/google/go-github/issues/comments{/number}","contents_url":"https://api.github.com/repos/google/go-github/contents/{+path}","compare_url":"https://api.github.com/repos/google/go-github/compare/{base}...{head}","merges_url":"https://api.github.com/repos/google/go-github/merges","archive_url":"https://api.github.com/repos/google/go-github/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/google/go-github/downloads","issues_url":"https://api.github.com/repos/google/go-github/issues{/number}","pulls_url":"https://api.github.com/repos/google/go-github/pulls{/number}","milestones_url":"https://api.github.com/repos/google/go-github/milestones{/number}","notifications_url":"https://api.github.com/repos/google/go-github/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/google/go-github/labels{/name}","releases_url":"https://api.github.com/repos/google/go-github/releases{/id}","deployments_url":"https://api.github.com/repos/google/go-github/deployments","created_at":"2013-05-24T16:42:58Z","updated_at":"2024-08-10T14:28:23Z","pushed_at":"2024-08-08T09:23:25Z","git_url":"git://github.com/google/go-github.git","ssh_url":"git@github.com:google/go-github.git","clone_url":"https://github.com/google/go-github.git","svn_url":"https://github.com/google/go-github","homepage":"https://pkg.go.dev/github.com/google/go-github/v63/github","size":6879,"stargazers_count":10234,"watchers_count":10234,"language":"Go","has_issues":true,"has_projects":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"has_discussions":false,"forks_count":2026,"mirror_url":null,"archived":false,"disabled":false,"open_issues_count":81,"license":{"key":"bsd-3-clause","name":"BSD 3-Clause \"New\" or \"Revised\" License","spdx_id":"BSD-3-Clause","url":"https://api.github.com/licenses/bsd-3-clause","node_id":"MDc6TGljZW5zZTU="},"allow_forking":true,"is_template":false,"web_commit_signoff_required":false,"topics":["github","github-api","go","golang","hacktoberfest"],"visibility":"public","forks":2026,"open_issues":81,"watchers":10234,"default_branch":"master","permissions":{"admin":false,"maintain":false,"push":false,"triage":false,"pull":true},"temp_clone_token":"","custom_properties":{},"organization":{"login":"google","id":1342004,"node_id":"MDEyOk9yZ2FuaXphdGlvbjEzNDIwMDQ=","avatar_url":"https://avatars.githubusercontent.com/u/1342004?v=4","gravatar_id":"","url":"https://api.github.com/users/google","html_url":"https://github.com/google","followers_url":"https://api.github.com/users/google/followers","following_url":"https://api.github.com/users/google/following{/other_user}","gists_url":"https://api.github.com/users/google/gists{/gist_id}","starred_url":"https://api.github.com/users/google/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/google/subscriptions","organizations_url":"https://api.github.com/users/google/orgs","repos_url":"https://api.github.com/users/google/repos","events_url":"https://api.github.com/users/google/events{/privacy}","received_events_url":"https://api.github.com/users/google/received_events","type":"Organization","site_admin":false},"network_count":2026,"subscribers_count":211}`)


Instead of an actual response, how about if you just put one value of each type that you found, please?

I minimized the json as you suggested to have a small partial project

gmlewis · 2024-08-10T16:56:52Z

Maybe @chapurlatn can review these changes?

codecov · 2024-08-10T16:57:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (2b8c7fa) to head (e6c19fd).
Report is 92 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3230      +/-   ##
==========================================
- Coverage   97.72%   92.94%   -4.78%     
==========================================
  Files         153      171      +18     
  Lines       13390    11633    -1757     
==========================================
- Hits        13085    10812    -2273     
- Misses        215      727     +512     
- Partials       90       94       +4

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

gmlewis · 2024-08-10T18:19:59Z

github/repos_test.go

+			wantErr:        false,
+		},
+		"With custom properties": {
+			data:           []byte(`{"custom_properties":{"boolean":"false","text":"a","single-select":"a","multi-select":["a","b","c"]}}`),


How about this?

Suggested change

data: []byte(`{"custom_properties":{"boolean":"false","text":"a","single-select":"a","multi-select":["a","b","c"]}}`),

data: []byte(`{"custom_properties":{"boolean":false,"int":42,"key":"value","array":["a","b","c"]}}`),

This would imply that there is a possible integer value when there is non (that we know of). I used the different custom property types offered by GitHub (see here) and observed their representation in the api, using their label in the github ui as key for this json.

I guess it depends on whats the goal - testing that the unmarshalling works according to the type or testing the possible values that github could send for the given field. What do you think?

Oh, I see, well at least please change the boolean from a string to false without the quotes.

GitHub returns the value for boolean type custom properties as strings ("true" or "false"). Should I remove the quotes regardless?

Are you sure? We have hundreds of endpoints that return booleans, and none of them are strings.
What makes you say this? You made a boolean custom property and it is returning a string?!?

Yes, I configured custom properties of all types and looked at the api response from github.
This is the actual response from the github api with one custom property (named accordingly) of each type configured:

{ // ... "custom_properties": { "type-multi-select": [ "a", "b" ], "type-single-select": "b", "type-text": "lala", "type-true-false": "true" }, //... }

I'd love it if they would return an actual boolean here 😄

OK, bizarre.
Then it looks like there is one linting failure remaining... let me look.

It looks like the generated files are out-of-date if you could please update them.

The whole integration of file generation is a really cool golang feature!
Just pushed the updated files 👍

gmlewis

Thank you, @spalberg !
LGTM

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

arymoraes

LGTM

And yeah I remember when I was doing #3200 that a boolean type custom property comes as string from the GitHub API.

gmlewis · 2024-08-11T01:36:44Z

Thank you, @arymoraes !
Merging.

spalberg added 2 commits August 10, 2024 16:28

fix: widen CustomProperties type to map[string]interface{}

c791785

added tests for json unmarshalling of projects

ca5985f

gmlewis requested changes Aug 10, 2024

View reviewed changes

gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Aug 10, 2024

gmlewis changed the title ~~fix: widen CustomProperties type to map[string]interface{} to align with github api~~ feat!: Widen CustomProperties type to map[string]interface{} to align with GitHub API Aug 10, 2024

integrated review suggestions

82b588f

gmlewis reviewed Aug 10, 2024

View reviewed changes

update generated files

e6c19fd

gmlewis approved these changes Aug 10, 2024

View reviewed changes

arymoraes approved these changes Aug 11, 2024

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 11, 2024

gmlewis merged commit 436e1fd into google:master Aug 11, 2024
6 of 7 checks passed

gmlewis mentioned this pull request Aug 21, 2024

Bump version of go-github to v64.0.0 #3240

Merged

This was referenced Aug 28, 2024

flux bootstrap github fails for repositories with custom properties fluxcd/flux2#4958

Closed

Fix GitHub bootstrap for repositories with custom properties fluxcd/flux2#4959

Merged

molnarm mentioned this pull request Dec 4, 2024

chore(deps): update module github.com/google/go-github/v62 to v67 bitrise-io/bitrise-webhooks#208

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Widen CustomProperties type to map[string]interface{} to align with GitHub API #3230

feat!: Widen CustomProperties type to map[string]interface{} to align with GitHub API #3230

spalberg commented Aug 10, 2024 •

edited by gmlewis

Loading

google-cla bot commented Aug 10, 2024

gmlewis left a comment

gmlewis Aug 10, 2024

spalberg Aug 10, 2024

gmlewis commented Aug 10, 2024

codecov bot commented Aug 10, 2024 •

edited

Loading

gmlewis Aug 10, 2024

spalberg Aug 10, 2024

gmlewis Aug 10, 2024

spalberg Aug 10, 2024

gmlewis Aug 10, 2024

spalberg Aug 10, 2024 •

edited

Loading

spalberg Aug 10, 2024 •

edited

Loading

gmlewis Aug 10, 2024

gmlewis Aug 10, 2024

spalberg Aug 10, 2024

gmlewis left a comment

arymoraes left a comment

gmlewis commented Aug 11, 2024

	data: []byte(`{"custom_properties":{"boolean":"false","text":"a","single-select":"a","multi-select":["a","b","c"]}}`),
	data: []byte(`{"custom_properties":{"boolean":false,"int":42,"key":"value","array":["a","b","c"]}}`),

feat!: Widen CustomProperties type to map[string]interface{} to align with GitHub API #3230

feat!: Widen CustomProperties type to map[string]interface{} to align with GitHub API #3230

Conversation

spalberg commented Aug 10, 2024 • edited by gmlewis Loading

google-cla bot commented Aug 10, 2024

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis commented Aug 10, 2024

codecov bot commented Aug 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalberg Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

spalberg Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

arymoraes left a comment

Choose a reason for hiding this comment

gmlewis commented Aug 11, 2024

spalberg commented Aug 10, 2024 •

edited by gmlewis

Loading

codecov bot commented Aug 10, 2024 •

edited

Loading

spalberg Aug 10, 2024 •

edited

Loading

spalberg Aug 10, 2024 •

edited

Loading