-
Notifications
You must be signed in to change notification settings - Fork 1
Create valkey resources using pgrator resources #331
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
base: main
Are you sure you want to change the base?
Conversation
9dcee7b to
136ed1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates Valkey provisioning and reconciliation in the API from Aiven aiven.io/v1alpha1 resources to nais.io/v1 Valkey resources (via pgrator/mapperator CRDs), while still listing/reading legacy Aiven Valkeys for compatibility.
Changes:
- Create/Update/Delete Valkeys using
github.com/nais/pgrator/pkg/api/v1(nais.io/v1) and add a dedicatedNaisValkeyWatcher. - Adjust Valkey conversion/mapping logic (tier/memory/maxMemoryPolicy) for the new CRD.
- Update Kubernetes scheme + fake client mappings and bump dependencies (including adding pgrator API module).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/persistence/valkey/queries.go | Switch CRUD to nais.io/v1 Valkey CRD, merge legacy + new watchers in reads/lists |
| internal/persistence/valkey/models.go | Add conversion from nais.io/v1 Valkey and adapt max memory policy parsing |
| internal/persistence/valkey/machines.go | Remove tier+memory → plan lookup now that plan is no longer set in created resources |
| internal/persistence/valkey/dataloader.go | Add nais.io/v1 GVR + NewNaisValkeyWatcher; add helper for NAIS dynamic client |
| internal/persistence/valkey/client.go | Simplify client struct now that watcher isn’t stored on it |
| internal/kubernetes/watchers/watchers.go | Register NaisValkeyWatcher in watcher setup |
| internal/kubernetes/watcher/watcher.go | Extend SystemAuthenticatedClient signature to accept options |
| internal/kubernetes/utils.go | Add helpers to convert to/from unstructured objects |
| internal/kubernetes/scheme.go | Register pgrator API schemes |
| internal/kubernetes/fake/fake.go | Register valkeys list kind for fake dynamic client |
| internal/cmd/api/http.go | Wire NaisValkeyWatcher into Valkey loader context |
| integration_tests/valkey_crud.lua | Update expected resources to nais.io/v1 Valkey CRD and remove Aiven ServiceIntegration assertions |
| go.mod / go.sum | Add pgrator API module and bump several dependencies |
Comments suppressed due to low confidence (1)
internal/kubernetes/watcher/watcher.go:316
- SystemAuthenticatedClient now accepts
opts ...ImpersonatedClientOption, but the implementation doesn’t forwardoptsto the underlying cluster watcher (it always calls watcher.SystemAuthenticatedClient(ctx) without options). This makes options like WithImpersonatedClientGVR ineffective. Suggestion: passopts...through towatcher.SystemAuthenticatedClient(ctx, opts...).
func (w *Watcher[T]) SystemAuthenticatedClient(ctx context.Context, cluster string, opts ...ImpersonatedClientOption) (dynamic.NamespaceableResourceInterface, error) {
for _, watcher := range w.watchers {
if watcher.cluster == cluster {
return watcher.SystemAuthenticatedClient(ctx)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Some features are therefore removed or altered, but most tests are fairly unchanged. Requires an update to liberator that's not merged yet: nais/liberator#306
6b92d05 to
3642d4b
Compare
Some features are therefore removed or altered, but most tests are fairly unchanged.
Requires an update to liberator that's not merged yet: nais/liberator#306