Add support for new custom properties for orgs APIs #3804
Conversation
…om properties in an enterprise
|
@stevehipwell did i implement organization_property support to enterprise rules correctly? |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
Thanks for the PR @Not-Dhananjay-Mishra, it's looking good. I've added some comments for you to follow up on.
@gmlewis I'd like to take a look at #3692 as well as the consistency of the custom property implementation before we release this code. If we're going to have a breaking change it'd make sense to make any other related changes at the same time.
|
@stevehipwell Thanks for the suggestions.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3804 +/- ##
==========================================
+ Coverage 92.27% 92.32% +0.04%
==========================================
Files 192 194 +2
Lines 13896 13984 +88
==========================================
+ Hits 12823 12911 +88
Misses 884 884
Partials 189 189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @Not-Dhananjay-Mishra and @stevehipwell!
LGTM.
@stevehipwell - we have a few options here:
Option A: Merge this and work on #3692 before cutting the next release
Option B: Work on #3692 before merging this
Now that I write these out, I'm betting you meant Option A, but please correct me if I'm wrong.
Either way, I will wait for your LGTM and reply before proceeding.
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
I think the naming here is hard, and a per my last comment I think the existing repository custom properties need some changes. But let's get the changes in this PR correct and then we only need to update the existing code.
In addition to my inline comment think the new files need to follow the pattern [enterprise|orgs]_organization_properties.go.
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @Not-Dhananjay-Mishra and @alexandear!
LGTM.
Merging.
|
Oh, sorry - @stevehipwell - I see you had comments - do you approve this PR now? |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
The new files haven't been renamed as requested in my previous review and the naming still isn't consistent.
github/orgs_custom_properties.go
Outdated
| // GitHub API docs: https://docs.github.com/rest/orgs/custom-properties-for-orgs#create-or-update-custom-property-values-for-an-organization | ||
| // | ||
| //meta:operation PATCH /organizations/{org}/org-properties/values | ||
| func (s *OrganizationsService) CreateOrUpdateOrgCustomPropertyValues(ctx context.Context, org string, values OrganizationCustomPropertyValues) (*Response, error) { |
There was a problem hiding this comment.
The name of this function doesn't match the function above organization/org.
There was a problem hiding this comment.
Thanks for suggestion, I have changed it to CreateOrUpdateOrganizationCustomPropertyValues. Do I need to change it to something else?
4 participants
Issue : #3799
This PR adds support for