-
Notifications
You must be signed in to change notification settings - Fork 2.2k
refactor!: Support array type for CustomProperty.DefaultValue field
#3740
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
Conversation
DefaultValue field (削除ここまで)CustomProperty.DefaultValue field (追記ここまで)
Codecov Report
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.11%. Comparing base (138500e) to head (6abfd62).
Additional details and impacted files
@@ Coverage Diff @@ ## master #3740 +/- ## ======================================= Coverage 91.11% 91.11% ======================================= Files 187 187 Lines 16697 16697 ======================================= Hits 15213 15213 Misses 1296 1296 Partials 188 188
☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.
🚀 New features to boost your workflow:
- ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@gmlewis
gmlewis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @zyfy29!
One minor tweak, please, and then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear
github/orgs_properties.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zyfy29 - for the a null to be passed through to the backend, we need to remove the omitempty.
Also, please make sure that there is a unit test where the value is nil (unassigned) so we demonstrate that the null is passed through to the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks!
@gmlewis
gmlewis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @zyfy29!
Could you please make sure all tests and linters pass by running step 4 of CONTRIBUTING.md?
zyfy29
commented
Sep 28, 2025
Oh, sorry, now it should be OK
@gmlewis
gmlewis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @zyfy29!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear
@stevehipwell
stevehipwell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I agree with the API design for this, I was planning on having this discussion on the issue when I had time to pick it up or someone else wanted to. This comment is to hold the PR while we discuss the implementation in #3692.
gmlewis
commented
Sep 29, 2025
I don't think I agree with the API design for this, I was planning on having this discussion on the issue when I had time to pick it up or someone else wanted to. This comment is to hold the PR while we discuss the implementation in #3692.
Ah, OK. Let's put this PR on hold then while it is being discussed.
Uh oh!
There was an error while loading. Please reload this page.
BREAKING CHANGE:
CustomProperty.DefaultValuewas*stringbut now isany.Fixes: #3692.
Fixes: #3741.