[FEAT] supports intercepting app on a local cluster#372
Conversation
Reviewer's Guide by SourceryThis pull request implements a new feature to support intercepting an app on a local cluster. The changes primarily involve adding a new method Sequence DiagramsequenceDiagram
participant Client
participant GraphQL
participant Domain
participant AppRepo
Client->>GraphQL: CoreInterceptAppOnLocalCluster
GraphQL->>Domain: InterceptAppOnLocalCluster
Domain->>Domain: canMutateResourcesInEnvironment
Domain->>AppRepo: Patch
AppRepo-->>Domain: Updated App
Domain->>Domain: applyApp
Domain-->>GraphQL: Result
GraphQL-->>Client: Result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @nxtcoder17 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a brief comment for the new InterceptAppOnLocalCluster function to explain its purpose and how it differs from the regular InterceptApp function.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| } | ||
|
|
||
| // InterceptApp implements Domain. | ||
| func (d *domain) InterceptAppOnLocalCluster(ctx ResourceContext, appName string, clusterName string, serviceIP string, intercept bool, portMappings []crdsv1.AppInterceptPortMappings) (bool, error) { |
There was a problem hiding this comment.
suggestion: Consider using a struct for function parameters
The function has a long parameter list. Consider creating a struct to encapsulate these parameters, which could improve readability and make it easier to extend the function in the future.
type InterceptAppParams struct {
Ctx ResourceContext
AppName string
ClusterName string
ServiceIP string
Intercept bool
PortMappings []crdsv1.AppInterceptPortMappings
}
func (d *domain) InterceptAppOnLocalCluster(params InterceptAppParams) (bool, error) {
| return true, nil | ||
| } | ||
|
|
||
| // InterceptApp implements Domain. |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the InterceptApp functions into a single, more flexible function with options.
The introduction of InterceptAppOnLocalCluster unnecessarily duplicates logic from InterceptApp. To reduce complexity and improve maintainability, consider refactoring these functions into a single, more flexible function. Here's a suggested approach:
func (d *domain) InterceptApp(ctx ResourceContext, appName string, intercept bool, options InterceptOptions) (bool, error) {
if err := d.canMutateResourcesInEnvironment(ctx); err != nil {
return false, errors.NewE(err)
}
patch := repos.Document{
fc.AppSpecInterceptEnabled: intercept,
}
if options.ToDevice != "" {
patch[fc.AppSpecInterceptToDevice] = options.ToDevice
}
if options.ToIPAddr != "" {
patch[fc.AppSpecInterceptToIPAddr] = options.ToIPAddr
}
if options.PortMappings != nil {
patch[fc.AppSpecInterceptPortMappings] = options.PortMappings
}
uApp, err := d.appRepo.Patch(ctx, ctx.DBFilters().Add(fields.MetadataName, appName), patch)
if err != nil {
return false, errors.NewE(err)
}
if err := d.applyApp(ctx, uApp); err != nil {
return false, errors.NewE(err)
}
return true, nil
}
type InterceptOptions struct {
ToDevice string
ToIPAddr string
PortMappings []crdsv1.AppInterceptPortMappings
}This refactoring combines both functions into a single, more flexible InterceptApp function. It uses an InterceptOptions struct to handle different intercept scenarios, including local cluster specifics. This approach reduces code duplication while maintaining the ability to handle different intercept types. You can then use this function for both regular and local cluster intercepts:
// For regular intercept
options := InterceptOptions{ToDevice: deviceName}
success, err := d.InterceptApp(ctx, appName, true, options)
// For local cluster intercept
options := InterceptOptions{ToDevice: clusterName, ToIPAddr: serviceIP, PortMappings: portMappings}
success, err := d.InterceptApp(ctx, appName, true, options)This refactoring simplifies the codebase, reduces duplication, and improves maintainability while preserving all functionality and allowing for future extensions.
cdfc1d0 to
c1d981d
Compare
[FEAT] supports intercepting app on a local cluster
Summary by Sourcery
Introduce functionality to intercept applications on a local cluster, including updates to the domain interface and GraphQL resolvers to support this feature.
New Features:
Enhancements: