This repository was archived by the owner on Jun 11, 2025. It is now read-only.
Feat/kloudlite edge clusters#367
Merged
Merged
Conversation
This feature is useful while self-hosting the platform, as it saves initial hassle of setting up sendgrid and everything prior to installing chart itself.
Reviewer's Guide by SourceryThis pull request introduces significant changes across multiple services and packages within the Kloudlite API, focusing on implementing edge cluster functionality, improving error handling, and refactoring various components. Key changes include the addition of platform edge cluster management, updates to gRPC services, and modifications to existing domain logic and data structures. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtcoder17 - I've reviewed your changes - here's some feedback:
Overall Comments:
- This is a significant and well-structured refactoring that improves the overall architecture. Please ensure that all new components and their interactions are well-documented, and consider updating any existing architecture diagrams to reflect these changes.
- The removal of container registry related code seems intentional, but it would be good to confirm that this functionality has been moved elsewhere or is no longer needed to avoid any unintended loss of features.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Comment on lines
+29
to
+90
| func (r *Repo) AllocatePlatformEdgeCluster(ctx context.Context, region string, account string) (*entities.PlatformEdgeCluster, error) { | ||
| m, err := r.AllocatedClusters.GroupByAndCount(ctx, repos.Filter{fc.ClusterAllocationClusterRegion: region}, fc.ClusterAllocationClusterName, repos.GroupByAndCountOptions{ | ||
| Limit: 1, | ||
| Sort: repos.SortDirectionAsc, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if len(m) > 1 { | ||
| return nil, errors.New("more than one cluster available") | ||
| } | ||
|
|
||
| var clusterName string | ||
|
|
||
| switch len(m) { | ||
| case 0: | ||
| { | ||
| x, err := r.EdgeClusters.FindOne(ctx, repos.Filter{fc.PlatformEdgeClusterRegion: region}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if x == nil { | ||
| return nil, mo_errors.ErrNoClustersInRegion | ||
| } | ||
|
|
||
| clusterName = x.Name | ||
|
|
||
| if _, err := r.AllocatedClusters.Create(ctx, &entities.ClusterAllocation{ | ||
| To: account, | ||
| Cluster: entities.ClusterAllocationClusterRef{ | ||
| Name: clusterName, | ||
| Region: region, | ||
| OwnedByAccount: x.OwnedByAccount, | ||
| PublicDNSHost: x.PublicDNSHostname, | ||
| }, | ||
| }); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| case 1: | ||
| { | ||
| for k := range m { | ||
| clusterName = k | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pec, err := r.EdgeClusters.FindOne(ctx, repos.Filter{ | ||
| fc.PlatformEdgeClusterName: clusterName, | ||
| fc.PlatformEdgeClusterRegion: region, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if pec == nil { | ||
| return nil, mo_errors.ErrNoClustersInRegion | ||
| } | ||
|
|
||
| return pec, nil |
There was a problem hiding this comment.
suggestion (performance): Consider improving error handling and performance in AllocatePlatformEdgeCluster
The current implementation uses GroupByAndCount for allocation, which may not scale well with a large number of clusters. Consider implementing a more efficient allocation strategy. Additionally, error handling could be more granular to differentiate between different types of failures (e.g., no clusters available vs. database errors).
func (r *Repo) AllocatePlatformEdgeCluster(ctx context.Context, region, account string) (*entities.PlatformEdgeCluster, error) {
cluster, err := r.findLeastAllocatedCluster(ctx, region)
if err != nil {
return nil, fmt.Errorf("finding least allocated cluster: %w", err)
}
if err := r.allocateCluster(ctx, cluster, account); err != nil {
return nil, fmt.Errorf("allocating cluster: %w", err)
}
return cluster, nil
}
nxtcoder17
force-pushed
the
feat/kloudlite-edge-clusters
branch
from
4a323f5 to
6802a6b
Compare
nxtcoder17
force-pushed
the
feat/kloudlite-edge-clusters
branch
from
1385f3e to
d5c1324
Compare
abdheshnayak
pushed a commit
that referenced
this pull request
Nov 5, 2024
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Summary by Sourcery
Introduce Kloudlite edge clusters and enhance the platform with new gRPC services for managing edge clusters and ensuring global VPN connections. Refactor the codebase for better modularity and improve logging and configuration handling. Update build and CI processes to accommodate new features and ensure robust testing.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: