This repository was archived by the owner on Jun 11, 2025. It is now read-only.
Feat/owned cluster#373
Merged
Merged
Conversation
added checks for owned cluster, while creating and listing
Reviewer's Guide by SourceryThis pull request introduces a new feature for owned clusters and implements app rollout on image push using a webhook. The changes span across multiple files in the infra and console applications, affecting cluster management, environment creation, and image registry functionality. Sequence DiagramsequenceDiagram
participant W as Webhook
participant C as WebhookConsumer
participant D as Domain
participant R as RegistryImageRepo
participant A as AppRepo
W->>C: Image Push Event
C->>D: UpsertRegistryImage
D->>R: Upsert Image
C->>D: RolloutAppsByImage
D->>A: Find Apps by Image
D->>D: Resync K8s Resources
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @abdheshnayak - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding logging or auditing for cluster ownership changes and access attempts to aid in debugging and security monitoring.
- Ensure that ownership checks in cluster creation and listing operations are performed atomically to prevent race conditions.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 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.
| return nil, ErrClusterAlreadyExists{ClusterName: cluster.Name, AccountName: ctx.AccountName} | ||
| } | ||
|
|
||
| if s, ok := cluster.GetLabels()[constants.ClusterLabelOwnedBy]; ok { |
There was a problem hiding this comment.
suggestion: New cluster ownership validation in CreateCluster
The ownership validation here is good. Consider extracting this logic into a separate function for reusability, as it might be needed in other parts of the codebase dealing with cluster operations.
func validateClusterOwnership(cluster *v1alpha1.Cluster, userId string) error {
if s, ok := cluster.GetLabels()[constants.ClusterLabelOwnedBy]; ok {
if s != userId {
return errors.Newf("provided wrong owner for cluster %q, expected %q", cluster.Name, userId)
}
}
return nil
}
if err := validateClusterOwnership(cluster, string(ctx.UserId)); err != nil {
return nil, err
}
abdheshnayak
added a commit
that referenced
this pull request
Nov 5, 2024
* ✨ Added checks for owned cluster added checks for owned cluster, while creating and listing * 🚧 Added owner checks to model * 🚧 Changed label key for owned by * 🐛 Fixed issues * 🚧 Changed to byok * ✨ Added app rollout * ✨ Added app rollout on image push
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.
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
Add support for owned clusters and implement application rollouts on image push. Enhance the infrastructure to handle BYOK clusters and improve volume management capabilities.
New Features:
Enhancements: