Skip to content
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

fix(website):Fix Reverse Proxy Deletion Failure in Certain Scenarios #7709

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

zhengkunwang223
Copy link
Member

Refs #7701

Copy link

f2c-ci-robot bot commented Jan 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.

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.

_ = fileOp.DeleteFile(backPath)
return updateNginxConfig(constant.NginxScopeServer, nil, &website)
}

func (w WebsiteService) OperateProxy(req request.WebsiteProxyConfig) (err error) {
var (
website model.Website
Copy link
Member

Choose a reason for hiding this comment

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

The provided code snippet contains several discrepancies that need to be addressed:

  1. Unmatched Line Numbers: The line numbers in the @@ directives do not match those in the code content. This can lead to incorrect highlighting of changes.

  2. Missing Imports: There is no import statement for files.NewFileOp() and getAppInstallByKey(). These should be included at the top of the file, typically near the beginning where other imports might be defined.

  3. Code Fragment Missing Functionality:

    • In DeleteProxy, there is a commented-out line _ = updateNginxConfig(constant.NginxScopeServer, nil, &website).
    • Similar comments apply in OperateProxy where functionality related to updating Nginx configuration might be missing if it isn't already handled elsewhere.
  4. Potential Security Issues: Removing proxy files without logging or auditing could leave open security risks. Ensure proper logging when deleting these files or implementing backup procedures before deletion.

  5. Documentation Gap: While the function signatures look sufficient, adding docstrings would enhance readability and help maintainability of the code base.

Here's an updated version with some basic improvements:

package website_service

import (
	"fmt"
)

// type definitions omitted for brevity

type WebsiteService struct {
	...

}

// Other methods remain unchanged ...

func (w *WebsiteService) DeleteProxy(req request.WebsiteProxyDel) error {
	// Add necessary logic here...
	err := fileOp.DeleteFile(path.Join(includeDir, req.Name+".conf"))
	if err != nil {
		return err
	}
	log.Info("Deleted Proxy File:", includePath)
	return nil
}

// Similarly, add log statements and other necessary checks for OperateProxy method

Ensure you have the appropriate imports and handle all potential edge cases such as directory existence checks before operations like reading or writing files. Adjust logging levels based on your application's needs.

}
helper.SuccessWithOutData(c)
}

// @Tags Website
// @Summary Update proxy conf
// @Accept json
Copy link
Member

Choose a reason for hiding this comment

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

No, there are no irregularities or significant issues with this code. The changes align well with standard RESTful API structure and security practices for handling delete requests related to websites' proxy configurations. However, here is a suggestion:

  • Ensure that the websiteService.DeleteProxy method actually handles errors correctly, especially if it interacts with external systems like databases or servers.
  • It may be beneficial to add more logging around critical sections of the function to improve debugging when things don't work as expected.

api: OperateProxyConfig,
params: proxyConfig,
api: DelProxy,
params: del,
});
};

Copy link
Member

Choose a reason for hiding this comment

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

The provided code has a few adjustments and optimizations that can be made. Here's a concise analysis:

  1. Imports: The GetProxyConfig import is not used anywhere in the component. You might want to remove it if it's no longer needed.

  2. Delete Proxy Function:

    • A new variable del is created with selected fields (id and name) from proxyConfig. This makes the call more targeted.
    • Ensure that both properties (id and name) are required when updating the state or sending data to the server because they seem to be required based on your error messages.
  3. Translation Messages: In the message for the delete operation, replace placeholder variables using ${variable} instead of [${'parameter'}], which aligns better with Vue.js templating syntax.

  4. Comments and Clarity: Add comments to explain non-obvious parts of the code, such as why certain methods are called or what their purpose is in context.

  5. Styling Considerations:

    • If there are additional styles defined elsewhere that you're expecting to use here, make sure these are correctly imported or linked.
    • Be cautious about introducing excessive DOM manipulation if this part of the UI changes significantly.

Here's an optimized version of the relevant section:

const openCreate = () => {...};

const openEditFile = (proxyConfig: Website.ProxyConfig) => {...};

const deleteProxy = async (proxyConfig: Website.ProxyConfig) => {
    // Create a clean object containing only necessary parameters
    const del = { id: proxyConfig.id, name: proxyConfig.name };

    opRef.value.acceptParams({
        title: i18n.global.t('commons.msg.deleteTitle'),
        names: [proxyConfig.name],
        msg: i18n.global.t('commons.msg.operatorHelper', [
            i18n.global.t('website.proxy'), 
            i18n.global.t('commons.button.delete')
        ]),
        api: DelProxy,
        params: del,  // Use this cleaned-up object for deletion
    });
};

By making these adjustments, the code will be clearer and potentially slightly faster due to reduced redundancy in parameter passing.

Copy link

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

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

/lgtm

@wanghe-fit2cloud
Copy link
Member

/approve

@f2c-ci-robot f2c-ci-robot bot added the lgtm label Jan 13, 2025
Copy link

f2c-ci-robot bot commented Jan 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

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

The pull request process is described here

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

@f2c-ci-robot f2c-ci-robot bot merged commit 99f62b7 into dev Jan 13, 2025
6 checks passed
@f2c-ci-robot f2c-ci-robot bot deleted the pr@dev@website branch January 13, 2025 15: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.

3 participants