Policy patch - leave fields unchanged if they're missing from request#64174
Conversation
Specifically this allows us avoid updating syntactic_indexing_enabled when we don't have the value available.
|
We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added. Sourcegraph teammates should refer to Accepting contributions for guidance. |
|
We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added. Sourcegraph teammates should refer to Accepting contributions for guidance. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added. Sourcegraph teammates should refer to Accepting contributions for guidance. |
|
The GitHub CLA Bot is rechecking to see that you have signed the CLA - note that it may take up to 30 minutes for your response to be synchronized. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added. Sourcegraph teammates should refer to Accepting contributions for guidance. |
|
The GitHub CLA Bot is rechecking to see that you have signed the CLA - note that it may take up to 30 minutes for your response to be synchronized. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added. Sourcegraph teammates should refer to Accepting contributions for guidance. |
|
The GitHub CLA Bot is rechecking to see that you have signed the CLA - note that it may take up to 30 minutes for your response to be synchronized. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added. Sourcegraph teammates should refer to Accepting contributions for guidance. |
|
The GitHub CLA Bot is rechecking to see that you have signed the CLA - note that it may take up to 30 minutes for your response to be synchronized. |
|
@cla-bot check |
|
The GitHub CLA Bot is rechecking to see that you have signed the CLA - note that it may take up to 30 minutes for your response to be synchronized. |
| retain_intermediate_commits = %s, | ||
| indexing_enabled = %s, | ||
| syntactic_indexing_enabled = %s, | ||
| syntactic_indexing_enabled = COALESCE(%s, p.syntactic_indexing_enabled), |
There was a problem hiding this comment.
Let's not push the nullability of the GraphQL API this deep. Afaict at the call site we have access to the previous record, so we should be able to do the merging in Go code
There was a problem hiding this comment.
Hm, this is the callsite - all we have is the constructed patch struct from the graphql args: https://github.com/sourcegraph/sourcegraph/pull/64174/files#diff-84bf2c19aa6cbb0438b9bbfb47f4eba71b981a3f4b9318f63d4f02895639a186R91-R100
unless I'm missing something?
There was a problem hiding this comment.
There was a problem hiding this comment.
🤦 how did I miss that
There was a problem hiding this comment.
Only issue is that it's not going to be atomic, but I seriously hope people don't blast policy update endpoint with concurrent requests
There was a problem hiding this comment.
It's wrapped in a transaction https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph@be17da7305215f5e28e9ec109f746739ddd3ac34/-/blob/internal/codeintel/policies/internal/store/configurations.go?L305, so it should be atomic
| type ConfigurationPolicyPatch struct { | ||
| ID int | ||
| RepositoryID *int | ||
| RepositoryPatterns *[]string | ||
| Name string | ||
| Type GitObjectType | ||
| Pattern string | ||
| Protected bool | ||
| RetentionEnabled bool | ||
| RetentionDuration *time.Duration | ||
| RetainIntermediateCommits bool | ||
| PreciseIndexingEnabled bool | ||
| SyntacticIndexingEnabled *bool | ||
| IndexCommitMaxAge *time.Duration | ||
| IndexIntermediateCommits bool | ||
| EmbeddingEnabled bool | ||
| } |
There was a problem hiding this comment.
nit-ish: Maybe we could extract the shared part into a struct and make the patch:
ConfigurationPolicyPatch = ConfigurationPolicyShared + Option[bool]
ConfigurationPolicyEntity = ConfigurationPolicyShared + bool
that should also let you avoid some of the spurious changes in policies/service.go by passing around the shared struct.
Using Option over * let's us be more explicit as well (some of the ptrs in here are not about optionality afaict
There was a problem hiding this comment.
My hope is that eventually (after a minor release possibly) we can remove the special status from syntactic indexing field, remove its optionality and either
- promote patch to actually useful struct by relaxing the API and making lots more fields optional
- remove patch if relaxing the API doesn't make sense
In case of (1) I would like to use options instead, yes.
kritzcreek
left a comment
There was a problem hiding this comment.
Blocking comment was addressed
Fixes GRAPH-782
Specifically this allows us avoid updating syntactic_indexing_enabled when we don't have the value available.
This is done to solve the problem where not providing that field crashes the resolver. I confirmed it by both via tests and via manual testing of the API
Test plan
Changelog