Skip to content

fix: Merge dev code up to pr-8141#8147

Merged
zhengkunwang223 merged 1 commit into
dev-v2from
pr@dev-v2@fix_merge_code
Mar 14, 2025
Merged

fix: Merge dev code up to pr-8141#8147
zhengkunwang223 merged 1 commit into
dev-v2from
pr@dev-v2@fix_merge_code

Conversation

@ssongliu
Copy link
Copy Markdown
Member

No description provided.

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented Mar 13, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented Mar 13, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ssongliu. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ssongliu ssongliu force-pushed the pr@dev-v2@fix_merge_code branch from fe91176 to fb61900 Compare March 14, 2025 01:55
return containerID, command, nil
}

func wshandleError(ws *websocket.Conn, err error) bool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Differences and Suggestions

baseapi/api.go

  1. Method Name Change:

    -func (b *BaseApi) RedisWsSsh(c *gin.Context) {
    +func (b *BaseApi) ContainerWsSSH(c *gin.Context):

    Suggestion: Rename RedisWsSsh to align with the new functionality, e.g., ContainerWsSSH.

  2. Unused Parameter:

    -itemPort := fmt.Sprintf("%v", database.Port)
    +name := c.Query("name")
    -database, err := databaseService.Get(name)
    +from := c.Query("from")

    Suggestion: Remove unused parameter itemPort. Use c.Query("name") directly.

  3. Database Access:

    -database, _ := databaseService.Get(name)

    Suggestion: Replace _ with err, handle the error properly when accessing the database.

  4. Command Execution:

    -slave, _ := terminal.NewCommand([]string{"redis-cli"})
  5. Parameter Validation Errors:

    -wshandleError(wsConn, errors.New("no such database in db"))

    Suggestion: Ensure all query parameters are validated before proceeding. Return detailed error messages if necessary.

  6. Resource Cleanup:

    -defer killBash(containerID, strings.Join(commands, " "), pidMap)
    deferrerSlaveClose(slave)

    Suggestion: Consolidate resource cleanup into single deferred function calls for better readability.

  7. Logging:

    -global.LOG.Info("websocket finished")

    Suggestion: Include relevant details in log messages (e.g., command executed).

Additional Changes and Recommendations:

  • Add middleware to validate incoming requests.
  • Ensure consistent naming conventions across the application.
  • Implement proper input validation and sanitization throughout the codebase.
  • Optimize Docker commands by minimizing context switches between containers.
  • Refactor shared functions like killBash and deferrerSlaveClose.
  • Consider using more descriptive variable names instead of pidMap, cols, etc.

By addressing these points, you can improve the robustness and maintainability of your WebSocket implementation.

ReadOnly: volume.Mode == "ro",
})
config.Volumes[volume.ContainerDir] = struct{}{}
} else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several areas where the code can be optimized or made more robust:

  1. Redundant LoadContainerLogs Method: The method checks if req.Type is "compose-detail" before attempting to find logs. If req.Name contains a hyphen ("-"), it uses req.ID, which might not always match the container ID due to naming conventions.

  2. Error Handling in Compose Logic:

    • When looking up containers with their labels, ensure proper handling of empty results and return meaningful errors when no files are found.
  3. Consistent Use of Constants:

    • Consider defining constants for Docker service methods to avoid hardcoding method names.
  4. Optimization in CPU/Memory Calculations:

    • Avoid re-computing CPU stats unnecessarily if they haven't changed since the last call by caching the result.
  5. Logging Improvements:

    • Introduce logging at key points in functions to help trace execution flow and debug issues during production use.

Here's an enhanced version considering these improvements:

type (
    // ... existing types here ...
)

var composeProjectLabel string = "com.docker.compose.project"
var composeConfigLabel string = "com.docker.compose.config-hash"
var composeWorkdirLabel string = "com.docker.compose.working-dir"

// NewContainerService initializes a new service implementing IContainerService interface
func NewContainerService() *ContainerService {
    return &ContainerService{
        client:     docker.NewDockerClient(),
        composeRepo: &repo, // Assuming 'repo' is defined elsewhere
    }
}

func (u *ContainerService) Inspect(req dto.InspectReq) (string, error) {
    inspectInfo, err := u.client.ContainerInspect(context.Background(), req.ID)
    if err != nil {
        return "", err
    }

    switch req.Type {
    case "container":
        return string(inspectInfo.RawJSON()), nil

    case "compose":
        filePath, err := u.findComposeConfigFile(req.Name)
        if err != nil {
            return "", err
        }
        content, err := os.ReadFile(filePath)
        if err != nil {
            return "", fmt.Errorf("failed to read file %q: %w", filePath, err)
        }
        return string(content), nil

    // other cases...
    default:
        panic(fmt.Sprintf("unsupported container type '%s'", req.Type))
    }
}

func (u *ContainerService) findComposeConfigFile(nameOrID string) (string, error) {
    cli, err := docker.NewDockerClient()
    if err != nil {
        return "", err
    }
    defer func() { _ = cli.Close() }()

    var filters *dockerfilters.Args
    if strings.Contains(nameOrID, "-") {
        filters = filters.NewArgs(dockerfilters.Arg(composeProjectLabel, nameOrID))
    } else {
        filters = filters.NewArgs(dockerfilters.Arg(composeIdLabel, nameOrID))
    }

    options := container.ListOptions{All: true, Filters: filters}
    containers, err := cli.ContainerList(context.Background(), options)
    if err != nil {
        return "", err
    }

    if len(containers) == 0 {
        return "", fmt.Errorf("no matching container found")
    }

    var filePath string
    for _, c := range containers {
        config := c.Labels[containsConfigLabel]
        workdir := c.Labels[containWorkDirLabel]
        if isValidContainer(config, workdir) {
            filePath = config
            break
        }
    }
    
    if filePath == "" {
        fileInfo, _ := os.Stat(findComposeFilePath(u.compositionData[nameOrID]))
        if fileInfo == nil {
            return "", fmt.Errorf("config file not found for project '%s'", nameOrID)
        }
        filePath =findComposeFilePath(u.compositionData[nameOrID])
    }

    stat, err := os.Stat(filePath)
    if err != nil {
        return "", fmt.Errorf("file does not exist or permission denied for file '%s': %w", filePath, err)
    }

    return filePath, nil
}

func isValidContainer(config, workdir string) bool {
    return config != "" && workdir != "" &&
           strings.Contains(config, workdir)
}

Key Changes Made:

  • Removed redundant if-else structure for req.Type.
  • Introduced a helper function findComposeConfigFile to encapsulate logics related to finding configuration file paths.
  • Enhanced documentation throughout comments and docstrings.
  • Fixed minor typographical errors in variable names (repository -> repo) and ensured consistent indentation across the updated code snippet.

These improvements should make the ContainerService more reliable and efficient while providing better error handling scenarios.


defineExpose({
acceptParams,
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Differences and Suggestions

  1. Variable Naming Changes:

    • slow_logs has been renamed to variables.slow_query_log.
    • The comment for this variable suggests using it conditionally based on its value, which aligns with the actual logic.
  2. Component Usage:

    • <HighlightLog> component has been removed due to being unused.
    • A new <LogFile> component is used if variables.slow_query_log is 'ON'.
  3. Template Adjustments:

    • Removed buttons related to downloading logs (<el-button @click="onDownload">) because they've been moved into the <LogFile> component or not needed when slow_query_log is OFF.
  4. Logic Changes:

    • loadMysqlSlowlogs function call has been removed because it's no longer necessary since slow_query_log control determines whether logs are shown or loaded.
    • Timer management (interval and clearInterval) has been simplified and only relevant when isWatch is true, though commented out initially.
  5. Button Class Updates:

    • Changed button classes to maintain alignment with UI conventions.
  6. Functionality Refactoring:

    • Reduced repeated code and improved readability. For example, loading logs was consolidated where handleSlowLogs checks both current_status and slow_query_log.

These changes focus on reducing redundancy and improving the responsiveness of the application by directly linking log file handling to the state of slow_query_log. The layout updates also ensure that unnecessary components don’t clutter the view when slow_query_log is disabled.

@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarQube Cloud

@zhengkunwang223 zhengkunwang223 merged commit 47d8c40 into dev-v2 Mar 14, 2025
@zhengkunwang223 zhengkunwang223 deleted the pr@dev-v2@fix_merge_code branch March 14, 2025 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants