-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
intercept app cli query updated #304
Conversation
Reviewer's Guide by SourceryThis pull request updates the CLI query for intercepting an app, introduces a new query for getting the DNS host suffix, and modifies the related GraphQL schema. The changes focus on adapting the app interception process for local clusters and adding functionality to retrieve DNS host information. Sequence DiagramsequenceDiagram
participant CLI
participant Server
participant LocalCluster
CLI->>Server: core_interceptAppOnLocalCluster(clusterName, ipAddr, ...)
Server->>LocalCluster: Intercept app
LocalCluster-->>Server: Interception result
Server-->>CLI: Return result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nxtcoder36 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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 and I'll use the feedback to improve your reviews.
@@ -293,24 +307,26 @@ export const cliQueries = (executor: IExecutor) => ({ | |||
), | |||
cli_interceptApp: executor( | |||
gql` | |||
mutation Core_interceptApp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider maintaining both device-based and cluster-based interception functions or creating a more generic function to handle both cases.
The changes to cli_interceptApp
(now cli_interceptAppOnLocalCluster
) introduce additional complexity by shifting from a device-based to a cluster-based approach. While this may be necessary for the evolving architecture, consider the following suggestions to minimize complexity and maintain API clarity:
- Keep both functions to ensure backwards compatibility:
cli_interceptApp: executor(
gql`
mutation Core_interceptApp(
$portMappings: [Github__com___kloudlite___operator___apis___crds___v1__AppInterceptPortMappingsIn!]
$intercept: Boolean!
$deviceName: String!
$appName: String!
$envName: String!
) {
core_interceptApp(
portMappings: $portMappings
intercept: $intercept
deviceName: $deviceName
appname: $appName
envName: $envName
)
}
`,
{
transformer: (data: any) => data.core_interceptApp,
vars: (_: any) => {},
}
),
cli_interceptAppOnLocalCluster: executor(
gql`
mutation Core_interceptAppOnLocalCluster(
$portMappings: [Github__com___kloudlite___operator___apis___crds___v1__AppInterceptPortMappingsIn!]
$intercept: Boolean!
$clusterName: String!
$ipAddr: String!
$appName: String!
$envName: String!
) {
core_interceptAppOnLocalCluster(
portMappings: $portMappings
intercept: $intercept
clusterName: $clusterName
ipAddr: $ipAddr
appname: $appName
envName: $envName
)
}
`,
{
transformer: (data: any) => data.core_interceptAppOnLocalCluster,
vars: (_: any) => {},
}
),
- If possible, consider creating a more generic function that can handle both device and cluster-based interception:
cli_interceptApp: executor(
gql`
mutation Core_interceptApp(
$portMappings: [Github__com___kloudlite___operator___apis___crds___v1__AppInterceptPortMappingsIn!]
$intercept: Boolean!
$target: InterceptTargetInput!
$appName: String!
$envName: String!
) {
core_interceptApp(
portMappings: $portMappings
intercept: $intercept
target: $target
appname: $appName
envName: $envName
)
}
`,
{
transformer: (data: any) => data.core_interceptApp,
vars: (_: any) => {},
}
),
Where InterceptTargetInput
is a new input type that can represent either a device or a cluster:
input InterceptTargetInput {
deviceName: String
clusterName: String
ipAddr: String
}
This approach would provide flexibility while maintaining a single, more generic API.
Please provide more context on why this change was necessary and how it fits into the broader architecture. This information would help in understanding the rationale behind the current implementation and potentially lead to more tailored suggestions for reducing complexity.
Summary by Sourcery
Introduce a new GraphQL query to fetch DNS host suffix and enhance the existing app interception mutation by renaming it and modifying its parameters to support local cluster interception.
New Features:
cli_getDNSHostSuffix
to retrieve the DNS host suffix.Enhancements:
cli_interceptApp
mutation tocli_interceptAppOnLocalCluster
, changing parameters fromdeviceName
toclusterName
and addingipAddr
.