feat(changelog): add the Changelog feature to track changes for API, ev…#319
feat(changelog): add the Changelog feature to track changes for API, ev…#319julius-malcovsky wants to merge 1 commit intomainfrom
Conversation
…ents etc Co-authored-by: Björn Kottner <[email protected]> Co-authored-by: Ismael Garba <[email protected]> Co-authored-by: Stefan Siber <[email protected]> Co-authored-by: Ron Gummich <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for the Changelog feature to track version history changes for APIs and Events. The implementation spans both the Rover controller plane and the Rover server, providing full CRUD operations, validation, and persistence capabilities.
Changes:
- Introduces new Changelog CRD with webhook validation for Kubernetes controller operations
- Implements changelog controller and handler for reconciliation logic
- Adds changelog server endpoints with file-based persistence and hash-based optimization
- Includes comprehensive test coverage for both webhook validation and server operations
- Updates OpenAPI specification and generated API code
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rover/README.md | Added Changelog CRD documentation |
| rover/api/v1/changelog_types.go | Changelog type definitions with validation markers |
| rover/internal/webhook/v1/changelog_webhook.go | Webhook validator for Changelog resources |
| rover/internal/webhook/v1/changelog_webhook_test.go | Comprehensive webhook validation tests |
| rover/internal/handler/changelog/handler.go | Handler for changelog state management |
| rover/internal/controller/changelog_controller.go | Controller for reconciliation |
| rover/cmd/main.go | Integration of changelog controller and webhook |
| rover/config/rbac/role.yaml | RBAC permissions for changelog resources |
| rover/config/crd/bases/rover.cp.ei.telekom.de_changelogs.yaml | CRD definition |
| rover-server/internal/controller/changelog.go | Server-side changelog operations |
| rover-server/internal/server/changelog_server.go | HTTP endpoints for changelog operations |
| rover-server/internal/server/server.go | Server configuration and routing |
| rover-server/pkg/store/stores.go | Store initialization |
| rover-server/internal/api/server.gen.go | Generated API code from OpenAPI spec |
| rover-server/api/openapi.yaml | OpenAPI specification |
| rover-server/go.mod | Dependency management updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| allOf: | ||
| - enum: | ||
| - API | ||
| - Event | ||
| - enum: | ||
| - API | ||
| - Event |
There was a problem hiding this comment.
The resourceType field in the Changelog CRD has a duplicate enum definition. Lines 54-59 contain the same enum constraints twice in an allOf structure. This is redundant and should be simplified to a single enum definition.
| allOf: | |
| - enum: | |
| - API | |
| - Event | |
| - enum: | |
| - API | |
| - Event | |
| enum: | |
| - API | |
| - Event |
| @@ -210,6 +218,13 @@ func main() { | |||
| os.Exit(1) | |||
| } | |||
| } | |||
| // nolint:goconst | |||
| if os.Getenv("ENABLE_WEBHOOKS") != "false" { | |||
| if err := webhookv1.SetupChangelogWebhookWithManager(mgr); err != nil { | |||
| setupLog.Error(err, "unable to create webhook", "webhook", "Changelog") | |||
| os.Exit(1) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
The webhook setup code in main.go contains three separate if os.Getenv("ENABLE_WEBHOOKS") != "false" blocks (lines 208-213, 215-220, 222-227) with the second and third ones marked with // nolint:goconst comments. This repetition could be consolidated into a single check with multiple webhook setups to improve maintainability and reduce duplication.
| return res, err | ||
| } | ||
|
|
||
| b, err := io.ReadAll(reader) |
There was a problem hiding this comment.
this isn't safe. you can allocate a lot of memory if Spec.Changelog will point to large file. It is better use reader directly with json.Decoder
There was a problem hiding this comment.
or if you want to keep it, it is better wrap reader into https://pkg.go.dev/io#LimitedReader
| func computeHash(data []byte) string { | ||
| hasher := sha256.New() | ||
| hasher.Write(data) | ||
| return base64.StdEncoding.EncodeToString(hasher.Sum(nil)) |
There was a problem hiding this comment.
why base64? standard way how to display hashes is hex format. https://pkg.go.dev/encoding/hex
| return base64.StdEncoding.EncodeToString(hasher.Sum(nil)) | ||
| } | ||
|
|
||
| var semverRegex = regexp.MustCompile(`^([0-9]+)\.([0-9]+)\.([0-9]+)$`) |
There was a problem hiding this comment.
this doesn't fully support semantic version. I recommend use external library
| valErr.AddInvalidError(field.NewPath("spec").Child("changelog"), "", "changelog file reference is required") | ||
| } | ||
|
|
||
| if changelog.Spec.Hash == "" { |
There was a problem hiding this comment.
this validation here can be more restricted, if you use hex format for hash
lukas016
left a comment
There was a problem hiding this comment.
using base64 for hash isn't standard, we should use hex format
…ents etc