Add Cody context filters to the site config and expose them via GraphQL#61101
Conversation
chwarwick
left a comment
There was a problem hiding this comment.
It is worth a little extra time up front to make sure you get the schema right to lock down the behavior you want to enforce. Since there is some time until the next release you could still make breaking changes but it's nice to avoid those if possible.
Also probably worth noting that those resolvers/settings are experimental and eventually dependent on Client versions >= X to not give the wrong impression.
keegancsmith
left a comment
There was a problem hiding this comment.
LGTM. I have high level design feedback with respect to the GraphQL. I would argue we shouldn't be modeling the shape of the config in graphql. I think we codyContextItems should return a string which is the marshalled JSON of the ignore rules. Two reasons:
- A client should always want all fields. A subset of fields is an invalid. IE the whole config needs to live together.
- Simplifies API / reduces scope of what we need to support w.r.t. API evolution.
However, I'm not a graphQL expert so I don't know if what I am saying is heresy.
Thanks for your review, @keegancsmith! We discussed it with @valerybugakov (thanks for your input, Valery!) and he pointed out that, on one hand, clients will always need an entire filters config, thus they won't benefit from the granularity of the current approach. On the other hand, having types defined on the API level is more reliable than parsing opaque JSON structures on the client to extract the rules. With the opaque JSON structure, it's still possible to parse it incorrectly and use a subset of fields, this approach does not really prevent bugs when the subset of rules is used. I also browsed the current GraphQL schema looking for similar cases. We return stringified JSON for the entire configurations (e.g., site config, settings, code host configs, etc.), but not for their subsets. It would probably be more consistent with the rest of the codebase to return explicit schema 🤔 I'm not a GraphQL expert either and I don't know the idiomatic way to handle such problems. I'm curious what other folks think of it. |
|
https://github.com/sourcegraph/sourcegraph/pull/61101/commits/3a3a8621d1aa5d44d61ffd4701aa9a247c2badca refactors to return the marshaled JSON of the ignore rules (as string). Context: |
| For details see https://sourcegraph.com/docs/cody/capabilities/ignore-context | ||
| """ | ||
| codyContextFilters( | ||
| version: Int! |
There was a problem hiding this comment.
@chwarwick, do you think version should be required? 🤔
There was a problem hiding this comment.
Yes, my thinking is that if the client is getting back a blob of json and the structure of that may change over time it should always be known what the version is. So if the client asks for v1 it should get v1 if it's possible to return.
By making it a required input we force the client to declare what version it can handle, and it gives us the opportunity to reject that tell the client it is out of date and can not be used.
…les-to-site-config
Closes https://github.com/sourcegraph/sourcegraph/issues/61605
Adds Cody context filters to the site config and corresponding GraphQL resolvers.
The
codyContextFiltersquery has a requiredversionparameter to simplify future API evolution. Returns "cody.contextFilters" site config param value as JSON.Example query
Test plan