sourcegraph/src-cli

src `list orgs` doesn't work with `first` parameter

leonore opened this issue · 3 comments

src orgs list -h suggests -first='-1' to list all orgs but the flag doesn’t actually work

11:18 sourcegraph (main) > SRC_ENDPOINT="https://demo.sourcegraph.com" SRC_ACCESS_TOKEN=${SG_DEMO_TOKEN} src orgs list -first='-1'

GraphQL errors: 17 errors occurred:
	* {
  "message": "you must provide a `first` or `last` value to properly paginate",
  "path": [
    "organizations",
    "nodes",
    9,
    "members",
    "nodes"
  ]
}

edit: upon further investigation this doesn't seem to work with any first values

Looked at this briefly, looks like this is a backend regression, src-cli still does what it's supposed to do. The new connection pagination work seems to disallow cursorless pagination (thus not allowing listing ALL anymore).
For backcompat, I think we should still allow this, with the default being "return all". This is where we currently throw:
https://sourcegraph.com/-/editor?remote_url=github.com%2Fsourcegraph%2Fsourcegraph&branch=main&file=cmd%2Ffrontend%2Fgraphqlbackend%2Fgraphqlutil%2Fconnection_resolver.go&editor=VSCode&version=2.2.14&start_row=120&start_col=5&end_row=122&end_col=2

We could also fix this in src-cli by using pagination internally to get all the results and then print those, but this won't revive older src-cli versions and potentially also might've broken customer scripts. I think the more long-term solution will be to revert that error msg for now, and then at some point implement proper pagination in src-cli and deprecate it properly in the API after, and in a potential 5.0 release or so we could start throwing that error again.
@thenamankumar what do you think about that?

@eseliger I think we should keep the check for long-term and for all other APIs as well.

To resolve this, we can make the following changes:

  • if first = -1, then we treat it as return all.
  • we can make orgs API default first to -1.

wdyt?

Long-term I fully agree but short-term it's a breaking change that broke all existing src-cli releases and potentially will break customer scripts that rely on the fact that not providing pagination options will return the whole list.
-1 is just a thing we do here in src-cli, it translates into first: null in the GraphQL request, so there's no way for older src-clis to indicate properly the want to get -1 entries.
The problem with -1 as a default for first is that it would then clash again in the backend if last is also set. We could do something like if first == -1 || first >0 && last >0 but that feels about as hacky as allowing pagination without a first for now, like we used to do.
We can (and should) start refactoring src-cli eventually to always use pagination (even when -1 is used and then page through all entries), and can deprecate it in the API, and remove in 5.0.
What do you think?