-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix: fix issue with add node do not restart docker #8838
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
Conversation
|
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. DetailsInstructions 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-sigs/prow repository. |
| withRestartDocker: boolean; | ||
| withDockerRestart: boolean; | ||
| } | ||
| } |
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.
It seems that there is an error in the line withRestartDocker when updating from version 738 to 752:
Change:
withRestartDocker:boolean;
To:
withDockerRestart:boolean|undefined = false ;
| withDockerRestart: withDockerRestart.value, | ||
| }) | ||
| .then(() => { | ||
| loading.value = false; |
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.
Your code appears to be mostly correct, but there is one minor issue: you have an extra line of withDockerRestart.value being printed to the console in the onBind function. This might cause unnecessary clutter or debugging output.
Here's a revised version of the submit function:
const submit = async () => {
loading.value = true;
try {
await bindLicense({
licenseID: form.licenseID,
nodeID: form.nodeID,
syncList: form.syncList,
withDockerRestart: withDockerRestart.value,
});
loading.value = false;
} catch (error) {
// Handle error here
}
};Summary of Changes:
- Removed Extra Console Log: Removed the unneeded
console.log(withDockerRestart.value);from theonBindfunction. - Optional Catch Block for Error Handling: Added a
try-catchblock within thesubmitfunction for better error handling.
| global.LOG.Info("restart docker") | ||
| restartCmd, err := getDockerRestartCommand() | ||
| if err != nil { | ||
| return err |
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.
The provided code differences have been reviewed, and no significant irregularities or potential issues were found. There are, however, a couple of optimizations that can be suggested:
-
Logging Initialization: Ensure that
global.LOGis properly initialized before it's used to log information about restarting Docker. This helps prevent runtime errors if logging hasn't been configured correctly.import "log" // assuming global.LOG is being used from some library or config // Initialize global.LOG at the beginning of the program or using appropriate configuration setup var global struct { LOG *log.Logger } func init() { global.LOG = log.Default() // Adjust path as according to your logger implementation }
-
Function Signature Changes: The function signature
getDockerRestartCommand()was modified to include an optional parameterwithErrorHandling. However, this change might lead to confusion without clear documentation on what this parameter accomplishes. It would be beneficial to document or explain its purpose. -
Variable Naming: The variable
req, which represents request data, should be named more descriptively, such assettingsRequest.
These changes improve clarity and maintainability in the codebase.
|
wanghe-fit2cloud
left a comment
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.
/lgtm
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |



No description provided.