Skip to content

fix!: Add ListCursorOptions to Enterprise and Org code security configs #3997

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.

Sign up for GitHub

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

Open
maishivamhoo123 wants to merge 2 commits into
base: master
Choose a base branch
from
+334 −19
Open

fix!: Add ListCursorOptions to Enterprise and Org code security configs #3997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e49a346
fix!: Add ListCursorOptions to Enterprise and Org code security configs
maishivamhoo123 Feb 12, 2026
72b1706
Merge branch 'master' into fix-enterprise-codesec-pagination
maishivamhoo123 Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions github/enterprise_codesecurity_configurations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,7 @@ import (
// supplied GitHub API will return an error. PerPage controls the number of items
// per page (max 100 per GitHub API docs).
type ListEnterpriseCodeSecurityConfigurationOptions struct {
// A cursor, as given in the Link header. If specified, the query only searches for security configurations before this cursor.
Before string `url:"before,omitempty"`

// A cursor, as given in the Link header. If specified, the query only searches for security configurations after this cursor.
After string `url:"after,omitempty"`

// For paginated result sets, the number of results to include per page.
PerPage int `url:"per_page,omitempty"`
Comment on lines -23 to -30
Copy link
Contributor

@alexandear alexandear Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmlewis maybe it is better to simply extend the gen-iterators.go to support generating iterator methods from the ListEnterpriseCodeSecurityConfigurationOptions?

Copy link
Contributor

@alexandear alexandear Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this kind of breaking change.

Copy link
Collaborator

@gmlewis gmlewis Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand, @alexandear - the point of using ListOptions and ListCursorOptions is to standardize the pagination handling. Since this endpoint uses cursor pagination, it makes sense to me to embed ListCursorOptions like we do with the other endpoints that support cursor pagination.
The next release is already going to have a large number of breaking API changes, mostly related to pagination, so I think it is fine to include this one.

Copy link
Contributor

@alexandear alexandear Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListCursorOptions contains more fields than the GitHub API expects. From a user's perspective, it's better to have a simpler ListEnterpriseCodeSecurityConfigurationOptions with only three fields: before, after, and per_page.

ListCursorOptions:

go-github/github/github.go

Lines 267 to 290 in 1732098

type ListCursorOptions struct {
// For paginated result sets, page of results to retrieve.
Page string `url:"page,omitempty"`
// For paginated result sets, the number of results to include per page.
PerPage int `url:"per_page,omitempty"`
// For paginated result sets, the number of results per page (max 100), starting from the first matching result.
// This parameter must not be used in combination with last.
First int `url:"first,omitempty"`
// For paginated result sets, the number of results per page (max 100), starting from the last matching result.
// This parameter must not be used in combination with first.
Last int `url:"last,omitempty"`
// A cursor, as given in the Link header. If specified, the query only searches for events after this cursor.
After string `url:"after,omitempty"`
// A cursor, as given in the Link header. If specified, the query only searches for events before this cursor.
Before string `url:"before,omitempty"`
// A cursor, as given in the Link header. If specified, the query continues the search using this cursor.
Cursor string `url:"cursor,omitempty"`
}

Copy link
Contributor

@alexandear alexandear Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, if we decide to use ListCursorOptions, let's simply inline it instead of ListEnterpriseCodeSecurityConfigurationOptions:

func (s *EnterpriseService) ListCodeSecurityConfigurations(ctx context.Context, enterprise string, opts *ListCursorOptions) ([]*CodeSecurityConfiguration, *Response, error) {

Copy link
Contributor Author

@maishivamhoo123 maishivamhoo123 Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexandear Sir Thank you for the suggestion.

I feel we should keep the wrapper struct as it is, rather than inlining ListCursorOptions.

The reason is that github/gen-iterators.go relies on the wrapper to generate the iterator. Specifically, the hasOptions function iterates over the Embeds list to find a match:
https://github.com/google/go-github/blob/master/github/gen-iterators.go#L177

Since ListCursorOptions defines fields directly and does not embed itself, hasOptions returns false if we use it directly in the function signature. This causes the generator to skip creating the Iter method for this endpoint entirely.

To support inlining, we would likely need to refactor the generator logic itself. Given this, and the benefit of future extensibility, I believe keeping the wrapper struct is the safer approach.

Please let me know if I have misunderstood the logic or if there is a better way to handle this.

Copy link
Contributor

@alexandear alexandear Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To support inlining, we would likely need to refactor the generator logic itself

You're correct.

Since we had already handled direct ListOptions, I added support for ListCursorOptions in #4007 for consistency.

Copy link
Collaborator

@gmlewis gmlewis Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @alexandear! I now see what you mean. 😂
Sounds good to me.

ListCursorOptions
}

// ListCodeSecurityConfigurations lists all code security configurations available in an enterprise.
Expand Down
17 changes: 15 additions & 2 deletions github/enterprise_codesecurity_configurations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@ import (

func TestEnterpriseService_ListCodeSecurityConfigurations(t *testing.T) {
t.Parallel()
opts := &ListEnterpriseCodeSecurityConfigurationOptions{Before: "1", After: "2", PerPage: 30}
opts := &ListEnterpriseCodeSecurityConfigurationOptions{
ListCursorOptions: ListCursorOptions{
Before: "1",
After: "2",
PerPage: 30,
},
}
ctx := t.Context()
client, mux, _ := setup(t)

Expand Down Expand Up @@ -391,7 +397,14 @@ func TestEnterpriseService_SetDefaultCodeSecurityConfiguration(t *testing.T) {

func TestEnterpriseService_ListCodeSecurityConfigurationRepositories(t *testing.T) {
t.Parallel()
opts := &ListCodeSecurityConfigurationRepositoriesOptions{Before: "1", After: "2", PerPage: 30, Status: "attached"}
opts := &ListCodeSecurityConfigurationRepositoriesOptions{
ListCursorOptions: ListCursorOptions{
Before: "1",
After: "2",
PerPage: 30,
},
Status: "attached",
}
ctx := t.Context()
client, mux, _ := setup(t)

Expand Down
93 changes: 93 additions & 0 deletions github/github-iterators.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

216 changes: 216 additions & 0 deletions github/github-iterators_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 1 addition & 8 deletions github/orgs_codesecurity_configurations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,7 @@ type ListOrgCodeSecurityConfigurationOptions struct {
// supplied GitHub API will return an error. PerPage controls the number of items
// per page (max 100 per GitHub API docs).
type ListCodeSecurityConfigurationRepositoriesOptions struct {
// A cursor, as given in the Link header. If specified, the query only searches for repositories before this cursor.
Before string `url:"before,omitempty"`

// A cursor, as given in the Link header. If specified, the query only searches for repositories after this cursor.
After string `url:"after,omitempty"`

// For paginated result sets, the number of results to include per page.
PerPage int `url:"per_page,omitempty"`
ListCursorOptions

// A comma-separated list of statuses. If specified, only repositories with these attachment statuses will be returned.
//
Expand Down
Loading
Loading