Draft
Conversation
github-actions
Bot
added
documentation
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds DigitalOcean App Platform as a new provider for Sablier, enabling automatic scaling of DigitalOcean apps based on demand. The implementation includes full provider functionality with start/stop operations, instance listing, status inspection, and event monitoring through polling.
Key Changes
- Implemented complete DigitalOcean provider with support for App Platform services and workers
- Added comprehensive integration tests requiring DigitalOcean API credentials
- Fixed grammatical errors in existing provider documentation ("knows" → "know")
- Added documentation and examples for the new provider
Reviewed Changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sabliercmd/provider.go | Added DigitalOcean provider initialization with token validation |
| pkg/provider/digitalocean/*.go | Implemented core provider functionality (start, stop, list, inspect, events) |
| pkg/provider/digitalocean/*_test.go | Added integration tests for all provider operations |
| pkg/config/provider.go | Added DigitalOcean configuration with token and region fields |
| docs/providers/digitalocean.md | Created comprehensive documentation for DigitalOcean provider |
| docs/providers/*.md | Fixed grammatical errors in existing documentation |
| docs/_sidebar.md | Added DigitalOcean to sidebar navigation (also added Nomad reference) |
| go.mod, go.sum | Added godo SDK and dependencies |
| README.md | Added DigitalOcean provider overview section |
| Makefile | Added gendocs target for documentation generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+95
to
+96
| ticker := time.NewTicker(5 * time.Second) | ||
| defer ticker.Stop() |
There was a problem hiding this comment.
The ticker is created but will continue ticking even after the function returns. Consider using defer ticker.Stop() after the ticker creation to ensure proper cleanup of resources.
| ) | ||
|
|
||
| func (p *Provider) InstanceStart(ctx context.Context, name string) error { | ||
| p.l.DebugContext(ctx, "starting app", "name", name) |
There was a problem hiding this comment.
Inconsistent logging style. Line 12 uses positional arguments "name", name while other log statements in the codebase use slog.String("name", name). For consistency, use slog.String("name", name) here as well.
Suggested change
| p.l.DebugContext(ctx, "starting app", "name", name) | |
| p.l.DebugContext(ctx, "starting app", slog.String("name", name)) |
Comment on lines
+29
to
+43
| // Scale up services | ||
| for i, service := range updateRequest.Spec.Services { | ||
| if service.InstanceCount == 0 { | ||
| updateRequest.Spec.Services[i].InstanceCount = 1 | ||
| needsUpdate = true | ||
| } | ||
| } | ||
|
|
||
| // Scale up workers | ||
| for i, worker := range updateRequest.Spec.Workers { | ||
| if worker.InstanceCount == 0 { | ||
| updateRequest.Spec.Workers[i].InstanceCount = 1 | ||
| needsUpdate = true | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The code iterates over services/workers twice - once to check if update is needed and once to actually scale. This could be simplified by doing both operations in a single loop, setting needsUpdate = true when scaling and only calling Apps.Update if needsUpdate is true at the end.
Suggested change
| // Scale up services | |
| for i, service := range updateRequest.Spec.Services { | |
| if service.InstanceCount == 0 { | |
| updateRequest.Spec.Services[i].InstanceCount = 1 | |
| needsUpdate = true | |
| } | |
| } | |
| // Scale up workers | |
| for i, worker := range updateRequest.Spec.Workers { | |
| if worker.InstanceCount == 0 { | |
| updateRequest.Spec.Workers[i].InstanceCount = 1 | |
| needsUpdate = true | |
| } | |
| } | |
| // Helper to scale up components | |
| scaleUp := func(components []godo.AppComponentSpec) bool { | |
| updated := false | |
| for i, c := range components { | |
| if c.InstanceCount == 0 { | |
| components[i].InstanceCount = 1 | |
| updated = true | |
| } | |
| } | |
| return updated | |
| } | |
| if scaleUp(updateRequest.Spec.Services) { | |
| needsUpdate = true | |
| } | |
| if scaleUp(updateRequest.Spec.Workers) { | |
| needsUpdate = true | |
| } |
| - [<img src="assets/img/docker.svg" height=24px width=24px />Docker](/providers/docker) | ||
| - [<img src="assets/img/docker_swarm.png" height=24px width=24px />Docker Swarm](/providers/docker_swarm) | ||
| - [<img src="assets/img/kubernetes.png" height=24px width=24px />Kubernetes](/providers/kubernetes) | ||
| - [<img src="assets/img/nomad.svg" height=24px width=24px />Nomad](/providers/nomad) |
There was a problem hiding this comment.
The sidebar references a Nomad provider on line 14, but no corresponding documentation file or implementation for Nomad is included in this PR. This appears to be an accidental addition. Either remove this line or add the corresponding Nomad documentation.
Suggested change
| - [<img src="assets/img/nomad.svg" height=24px width=24px />Nomad](/providers/nomad) |
|
|
||
| return &Provider{ | ||
| Client: client, | ||
| desiredReplicas: 1, |
There was a problem hiding this comment.
The desiredReplicas field is hardcoded to 1 and never used elsewhere. Consider removing this field if it's not needed, or document why it exists if it's intended for future use.
Comment on lines
+51
to
+52
| if lastCount, exists := lastState[app.ID]; exists { | ||
| if lastCount > 0 && currentCount == 0 { |
There was a problem hiding this comment.
The condition lastCount > 0 && currentCount == 0 will only detect transitions from running to stopped. If an app was already at 0 instances when Sablier started, changes won't be detected. Consider if this is the intended behavior or if initial state tracking is needed.
| return podman.New(connText, logger) | ||
| case "digitalocean": | ||
| if config.DigitalOcean.Token == "" { | ||
| return nil, fmt.Errorf("Digital Ocean token is required") |
There was a problem hiding this comment.
The error message should maintain consistent capitalization. "Digital Ocean" should be "DigitalOcean" (one word) to match the casing used throughout the codebase and the provider name.
Suggested change
| return nil, fmt.Errorf("Digital Ocean token is required") | |
| return nil, fmt.Errorf("DigitalOcean token is required") |
Comment on lines
+41
to
+42
| // Region is the Digital Ocean region. Defaults to "nyc1" | ||
| Region string `mapstructure:"REGION" yaml:"region,omitempty" default:"nyc1"` |
There was a problem hiding this comment.
The Region field is defined in the configuration but never actually used in the provider implementation. Either remove this field or implement the intended functionality. The comment on line 95 in the documentation mentions "this setting may be used for future region-specific features" suggesting it's not currently functional.
Comment on lines
+19
to
+21
| p.l.DebugContext(ctx, "app inspected", | ||
| slog.String("app", name), | ||
| slog.String("phase", string(app.ActiveDeployment.Phase)), |
There was a problem hiding this comment.
Potential nil pointer dereference. If app.ActiveDeployment is nil, accessing app.ActiveDeployment.Phase on line 21 will cause a panic. Add a nil check before accessing the Phase field.
Suggested change
| p.l.DebugContext(ctx, "app inspected", | |
| slog.String("app", name), | |
| slog.String("phase", string(app.ActiveDeployment.Phase)), | |
| phase := "N/A" | |
| if app.ActiveDeployment != nil { | |
| phase = string(app.ActiveDeployment.Phase) | |
| } | |
| p.l.DebugContext(ctx, "app inspected", | |
| slog.String("app", name), | |
| slog.String("phase", phase), |
Comment on lines
+43
to
+48
| for _, service := range app.Spec.Services { | ||
| currentReplicas += int32(service.InstanceCount) | ||
| } | ||
| for _, worker := range app.Spec.Workers { | ||
| currentReplicas += int32(worker.InstanceCount) | ||
| } |
There was a problem hiding this comment.
[nitpick] Duplicate code: The logic for counting instances from services and workers (lines 43-48) is duplicated from lines 28-35. Consider extracting this into a helper function to reduce code duplication.
❌ 6 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Digital Ocean has sponsored this project!
An attempt to create a DigitalOcean provider and running a sablier instance on DigitalOcean will be added.