Skip to content
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 in CreateOrUpdateRepoCustomPropertyValues? #3021

Closed
peter-aglen opened this issue Dec 6, 2023 · 3 comments · Fixed by #3023
Closed

Bug in CreateOrUpdateRepoCustomPropertyValues? #3021

peter-aglen opened this issue Dec 6, 2023 · 3 comments · Fixed by #3023
Assignees
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). bug

Comments

@peter-aglen
Copy link
Contributor

We are looking to replace our custom-built methods for listing and updating custom property values, with the methods provided in v57.

It seems the CreateOrUpdateRepoCustomPropertyValues method has a bug however. We haven't actually tested it yet, but shouldn't the last argument be properties []*CustomPropertyValue?

// Wrong type in last arg?
func (s *OrganizationsService) CreateOrUpdateRepoCustomPropertyValues(ctx context.Context, org string, repoNames []string, properties []*CustomProperty) (*Response, error) {
	u := fmt.Sprintf("orgs/%v/properties/values", org)

	params := struct {
		RepositoryNames []string          `json:"repository_names"`
		Properties      []*CustomProperty `json:"properties"`
	}{
		RepositoryNames: repoNames,
		Properties:      properties,
	}

	req, err := s.client.NewRequest("PATCH", u, params)
	if err != nil {
		return nil, err
	}

	return s.client.Do(ctx, req, nil)
}
@gmlewis
Copy link
Collaborator

gmlewis commented Dec 6, 2023

Looking at the corresponding unit test: https://github.com/google/go-github/blob/master/github/orgs_properties_test.go#L350
I see that it does not match the signature of the official docs, so you are absolutely correct.

I apologize for letting this slip past code reviews.

Would you like to create a PR to fix this, or shall I open it up to other contributors to our repo?

@gmlewis gmlewis added bug Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Dec 6, 2023
@peter-aglen
Copy link
Contributor Author

Sure, I can have a PR ready tomorrow.

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 6, 2023

Thank you, @peter-aglen !
It's yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants