-
Notifications
You must be signed in to change notification settings - Fork 73
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
test #205
base: master
Are you sure you want to change the base?
test #205
Conversation
Signed-off-by: Cyber-SiKu <[email protected]>
Signed-off-by: Cyber-SiKu <[email protected]>
With database disk records commited by `disks.yaml`, disk formatting cloud be performed without `format.yaml`. And it will get and wirte disk `size` and `URI` during disk formatting, thus record the ID of service(chunkserver) with associated disk when deploy curvebs cluster. Use `curveadm disks ls` to view all disk information. Usage: curveadm disks commit /path/to/disks.yaml curveadm disks ls curveadm format Signed-off-by: Lijin Xiong <[email protected]>
Feature: support managing disk information in database
@@ -264,6 +285,8 @@ func (curveadm *CurveAdm) Err() io.Writer { return curveadm.e | |||
func (curveadm *CurveAdm) Storage() *storage.Storage { return curveadm.storage } | |||
func (curveadm *CurveAdm) MemStorage() *utils.SafeMap { return curveadm.memStorage } | |||
func (curveadm *CurveAdm) Hosts() string { return curveadm.hosts } | |||
func (curveadm *CurveAdm) Disks() string { return curveadm.disks } | |||
func (curveadm *CurveAdm) DiskRecords() []storage.Disk { return curveadm.diskRecords } | |||
func (curveadm *CurveAdm) ClusterId() int { return curveadm.clusterId } | |||
func (curveadm *CurveAdm) ClusterUUId() string { return curveadm.clusterUUId } | |||
func (curveadm *CurveAdm) ClusterName() string { return curveadm.clusterName } |
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 code patch adds two new variables (disks and diskRecords). It also added two functions to get the disks and disk records.
The code looks good and appears to be properly formatted. The only issue I see is that the variables disks and diskRecords are not declared as constants, which might lead to confusion when a user is modifying the code. To make the code easier to read, I would suggest declaring the variables as constants. Additionally, it would be helpful to include comments for each function to explain what it does.
@@ -61,6 +62,7 @@ func addSubCommands(cmd *cobra.Command, curveadm *cli.CurveAdm) { | |||
cluster.NewClusterCommand(curveadm), // curveadm cluster ... | |||
config.NewConfigCommand(curveadm), // curveadm config ... | |||
hosts.NewHostsCommand(curveadm), // curveadm hosts ... | |||
disks.NewDisksCommand(curveadm), // curveadm disks ... | |||
playground.NewPlaygroundCommand(curveadm), // curveadm playground ... | |||
target.NewTargetCommand(curveadm), // curveadm target ... | |||
pfs.NewPFSCommand(curveadm), // curveadm pfs ... |
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.
with a basic code review
- The patch adds a new command, "curveadm disks ...". This should be a welcome addition and make the cli more user friendly.
- It is important to ensure that all input validation is done properly and that any data stored in the cli is done in a secure way.
- It would be useful to add some unit tests for the new command, as well as adding any necessary documentation.
- Any logging should be done in a way that is not intrusive and does not overwhelm the user.
NewListCommand(curveadm), | ||
) | ||
return cmd | ||
} |
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 code review.
- The first thing that should be done is an indentation check to make sure the code is properly formatted.
- It would be helpful if the author provided more comments within the code to explain the logic of the code.
- It is good practice to check for any unused variables or imports that are not needed in the code.
- Additionally, it would be wise to check for any possible bugs or errors that may occur when running the code.
- Additionally, it is important to check for any security risks that may arise from the code.
output := tui.FormatDisks(diskRecords) | ||
curveadm.WriteOut(output) | ||
return nil | ||
} |
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 code review.
- The code looks well-structured, with good indentation and comments.
- The code should be consolidated into a function to improve readability and maintainability.
- The code should use error check to make sure that all operations are successful.
- There should be more descriptive variable names in order to make the code easier to read and understand.
- The code should use a consistent style of formatting so that it is easier to read and understand.
- The code should use proper logic to ensure that the correct data is retrieved.
curveadm.WriteOut(disks) | ||
} | ||
return nil | ||
} |
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.
with a brief code review.
The code looks well organized and clear, following the Apache License 2.0. It also contains comments to explain the purpose of the code which is helpful for anyone reading it. However there a few things that can be improved:
- It would be helpful to use a consistent coding style, especially for indentation, throughout the code.
- If any variables are used in this code, it would be helpful to use descriptive variable names to make the code easier to read.
- If applicable, it would be helpful to add more error handling logic to improve the robustness of the code.
- It would be helpful to add some unit tests to ensure that the code works as expected.
continue | ||
} | ||
fcs = append(fcs, fc) | ||
} | ||
} | ||
|
||
// 2) generate start playbook |
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 code review.
Firstly, it is a good practice to include comments for each function and its purpose. Also, you should add an explanatory comment for each variable.
Secondly, in order to make the code more readable, you can split the code into smaller blocks by using functions.
Thirdly, you should use type assertions when handling different types of data.
Fourthly, you should add error handling for each block of code.
Finally, you should add comments to explain why a certain code was written.
if len(curveadm.DiskRecords()) == 0 { | ||
continue | ||
} | ||
configs = curveadm.FilterDeployConfigByRole(dcs, ROLE_CHUNKSERVER) | ||
} | ||
|
||
pb.AddStep(&playbook.PlaybookStep{ |
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.
with the code review
This code patch looks good overall. The extra CHECK_ITEM_DISK variable was added to track disk size, and the related logic in genPrecheckPlaybook was also adjusted accordingly. The code is well-structured and easy to read.
However, there are some potential areas of improvement:
- It would be useful to add a comment explaining why the disk size checking is skipped when the disk records are empty
- The variable names should be more descriptive and reflect the purpose of the variable, e.g. CHECK_ITEM_DISK_SIZE instead of CHECK_ITEM_DISK
- The code should be tested for edge cases to ensure it works correctly
mount: /data/chunkserver3 | ||
host: | ||
- curve-1 | ||
- curve-2 |
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.
-
Naming consistency of the variables:
The variable names used in the patch should be consistent throughout, e.g.global
anddisk
should use the same name format. -
Formatting of the code:
The code should be properly formatted with indentations and line breaks where appropriate. -
Comments:
Comments should be added to explain the purpose of the code and any assumptions made. -
Bug Risk:
There is potential for bugs to occur due to typos or incorrect formatting of the code, for example if theexclude
statement is not formatted correctly it could cause unexpected behaviour. -
Improvement Suggestion:
It would be beneficial to include a validation step to ensure that all the variables are correctly formatted before running the code.
DEBUG_SMART_CONFIGS = "DEBUG_SMART_CONFIGS" | ||
DEBUG_TOPOLOGY = "DEBUG_TOPOLOGY" | ||
DEBUG_TOOL = "DEBUG_TOOL" | ||
DEBUG_CLUSTER = "DEBUG_CLUSTER" | ||
DEBUG_CREATE_POOL = "DEBUG_CREATE_POOL" | ||
DEBUG_CLIENT_CONFIGURE = "DEBUG_CLIENT_CONFIGURE" | ||
DEBUG_CLIENT_CONFIGURE = "DEBUG_CLIENT_CONFIGURE" | ||
) | ||
|
||
type Field struct { |
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 code review.
First of all, you have added a constant DEBUG_DISKS which is great but it has not been used anywhere in the code. You should make use of this constant or remove it as it will add to the compilation time.
Second, you have changed DEBUG_CLIENT_CONFIGURE from a constant to a variable. This may cause unexpected behavior if not handled properly. You should make sure that any variables that are used with this constant are initialized and have the right values before you use them.
Lastly, it looks like you forgot to update the comment above the Field struct. You should make sure to update the comments so that other developers know what the code is doing.
Overall, the code looks good. There are some minor changes that you should make, but nothing major.
|
||
DISK_FORMAT_MOUNT_POINT = "mount" | ||
DISK_FORMAT_CONTAINER_IMAGE = "container_image" | ||
|
||
// format | ||
KEY_ALL_FORMAT_STATUS = "ALL_FORMAT_STATUS" | ||
|
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.
with a brief code review.
- The code patch adds constants for disk related operations, such as DISK_DEFAULT_NULL_SIZE and DISK_EXCLUDE_HOST. These constants could help to improve readability of the code and make it easier to maintain.
- The patch also adds constants for various disk filters and formats. This will help to keep track of the different disk operations and will help in debugging any issues that may arise.
- The patch adds constants for various disk formats, such as DISK_FORMAT_PERCENT and DISK_FORMAT_MOUNT_POINT. This will help to keep track of the different disk formats and will make it easier to maintain the code.
- The patch also adds constants for various disk operations, such as DISK_FILTER_ALL and DISK_FILTER_HOST. This will help to better organize the code and make it easier to maintain.
Overall, the code patch looks good and should help improve the readability and maintainability of the code. However, it is always a good idea to test the changes thoroughly to ensure that there are no bugs introduced by this patch.
slice = append(slice, str.(T)) | ||
} | ||
return slice | ||
} |
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 review.
Overall, the code looks like it is well-structured and easy to read. The addition of a new case for REQUIRE_SLICE seems to be handled correctly with proper checks in place. There are some minor issues with the code though.
Firstly, the function should have a comment explaining what it does and why it is needed. Secondly, in the REQUIRE_SLICE case, there is no check for empty slices and it directly calls convertSliceT with an empty slice which will cause a panic. To fix this, an if statement can be used to check for empty slices. Lastly, the convertSliceT function is declared outside the Build function which is not necessary. It can be declared inside the Build function.
Apart from these issues, the code looks good.
func (dc *DiskConfig) GetHost() []string { return dc.getStrSlice(CONFIG_GLOBAL_HOST) } | ||
func (dc *DiskConfig) GetDevice() string { return dc.getString(CONFIG_DISK_DEVICE) } | ||
func (dc *DiskConfig) GetMountPoint() string { return dc.getString(CONFIG_DISK_MOUNT_POINT) } | ||
func (dc *DiskConfig) GetHostExclude() []string { return dc.getStrSlice(CONFIG_DISK_HOST_EXCLUDE) } |
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.
with the code review:
- The code is generally well written and follows the Go coding conventions.
- The variable names are informative enough to understand the purpose of the code.
- There are no unnecessary variables or functions declared in the code.
- The appropriate data types have been used for each variable.
- The code is properly indented and the comments are helpful in understanding the purpose of the code.
- There are some potential bugs that could arise from using the getString(), getInt() and getStrSlice() functions as they only return a value when it is not nil. It might be better to also check if the value is an empty string or empty array.
- The function names are not very descriptive and can be made more specific.
- Error handling has not been implemented in the code.
false, | ||
nil, | ||
) | ||
) |
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.
with the code review.
Overall, the code looks good and all of the code is well commented. The code seems to be setting up variables related to disks. One suggestion I have is that it might be a good idea to break the code into multiple functions for better readability and maintainability. Also, some of the variable names could be more descriptive. Other than that, the code looks good.
@@ -121,7 +122,7 @@ func ParseFormat(filename string) ([]*FormatConfig, error) { | |||
fcs := []*FormatConfig{} | |||
for _, host := range format.Hosts { | |||
for _, disk := range format.Disks { | |||
fc, err := newFormatConfig(containerImage, host, disk) | |||
fc, err := NewFormatConfig(containerImage, host, disk) | |||
if err != nil { | |||
return nil, 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.
from code review
- The function newFormatConfig() is renamed to NewFormatConfig(). The first letter of a function name should be capitalized in golang.
- A new field UseDiskUri is added, it should have a proper initial value.
- The input parameter disk should have a proper format check before using.
func (dc *DeployConfig) GetReportUsage() bool { return dc.getBool(CONFIG_REPORT_USAGE) } | ||
func (dc *DeployConfig) GetContainerImage() string { | ||
return dc.getString(CONFIG_GLOBAL_CONTAINER_IMAGE) | ||
} | ||
func (dc *DeployConfig) GetLogDir() string { return dc.getString(CONFIG_LOG_DIR) } | ||
func (dc *DeployConfig) GetDataDir() string { return dc.getString(CONFIG_DATA_DIR) } | ||
func (dc *DeployConfig) GetCoreDir() string { return dc.getString(CONFIG_CORE_DIR) } |
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 code review for this patch:
- The code looks syntactically correct and clear. The indentation is also consistent.
- It looks like the patch is updating the GetContainerImage() function to use the CONFIG_GLOBAL_CONTAINER_IMAGE instead of CONFIG_CONTAINER_IMAGE. It would be good to make sure that this is the intended behavior.
- The names of the variables and functions are descriptive and easy to understand.
- It would be good to add additional tests to ensure that the function works as expected with the updated parameters.
- Since the patch is modifying an existing function, it would be good to also check if there are any potential impacts on other parts of the code.
@@ -96,7 +98,7 @@ var ( | |||
DEFAULT_REPORT_USAGE, | |||
) | |||
|
|||
CONFIG_CONTAINER_IMAGE = itemset.insert( | |||
CONFIG_GLOBAL_CONTAINER_IMAGE = itemset.insert( | |||
"container_image", | |||
REQUIRE_STRING, | |||
true, |
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.
from the first part, the code add a new item REQUIRE_SLICE in line 6.
In line 69, the comment has been modified to make it more clear for user to understand.
Line 96-98, a new item CONFIG_GLOBAL_CONTAINER_IMAGE is inserted into itemset.
Overall, the code looks correct with proper indentation and formatting. No bug risk found, but you can consider adding appropriate comments to explain the codes.
@@ -175,6 +176,8 @@ func (p *Playbook) createTasks(step *PlaybookStep) (*tasks.Tasks, error) { | |||
t, err = checker.NewGetHostDate(curveadm, config.GetDC(i)) | |||
case CHECK_HOST_DATE: | |||
t, err = checker.NewCheckDate(curveadm, nil) | |||
case CHECK_DISK_SIZE: | |||
t, err = checker.NewCheckDiskSizeTask(curveadm, config.GetDC(i)) | |||
case CHECK_CHUNKFILE_POOL: | |||
t, err = checker.NewCheckChunkfilePoolTask(curveadm, config.GetDC(i)) | |||
case CHECK_S3: |
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 code review!
The code patch looks to add a new task, CHECK_DISK_SIZE. This should be good to go. The only thing I would suggest is to add a comment above the task definition which describes what the task does. This will make it easier to understand what the task is trying to accomplish. Additionally, it may be a good idea to add some logging statements to the task so that you can debug any potential issues with it.
DELETE_DISK_HOST = `DELETE from disk WHERE host = ?` | ||
|
||
DELETE_DISK_HOST_DEVICE = `DELETE from disk WHERE host = ? AND device = ?` | ||
|
||
// cluster | ||
INSERT_CLUSTER = `INSERT INTO clusters(uuid, name, description, topology, pool, create_time) | ||
VALUES(hex(randomblob(16)), ?, ?, ?, "", datetime('now','localtime'))` |
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 code review:
- The patch adds two new tables, disks and disk, to store data related to disks. It appears that all the necessary fields are included in the CREATE TABLE statements.
- It also adds a few SELECT and DELETE statements to query and delete data related to disks.
- The INSERT and SET statements appear to be complete, although it would be beneficial to add some comments to explain the purpose of each statement.
- The code formatting is inconsistent, which can make it difficult to read.
- There are no error handling or input validation checks, which could put the application at risk.
device: device, | ||
curveadm: curveadm, | ||
}) | ||
} | ||
// 3: run container to format chunkfile pool | ||
t.AddStep(&step.PullImage{ | ||
Image: fc.GetContainerImage(), |
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.
with a brief code review:
- The code looks like it adds an additional step to a task that formats a chunkfile pool. This additional step appears to update the disk size and disk URI of the device used in the formatting process.
- This code is adding an additional struct, step2UpdateDiskSizeUri, which appears to take a host, device, size, diskId, and curveadm as parameters. This step will then use methods from curveadm.Storage() to update the disk size and disk URI.
- The code also adds a checkDeviceUUID lambda, which appears to check if the device has a valid UUID.
- Finally, the code adds a condition to the NewFormatChunkfilePoolTask to add the step2UpdateDiskSizeUri step if UseDiskUri is set to true.
In terms of bug risk, it appears that this code is adding additional steps to an existing task, so it is likely safe to add. However, it is possible that there could be bugs related to the updating of the disk size and disk URI, so it would be wise to test this code thoroughly before deploying it.
In terms of improvement suggestions, it might be worth considering adding validation checks to ensure that the parameters passed to the step2UpdateDiskSizeUri step are valid. Additionally, it might be beneficial to add logging statements to track the progress of this step.
|
||
return t, nil | ||
} | ||
|
||
func NewCheckS3Task(curveadm *cli.CurveAdm, dc *topology.DeployConfig) (*task.Task, error) { | ||
subname := fmt.Sprintf("host=%s role=%s", dc.GetHost(), dc.GetRole()) | ||
t := task.NewTask("Check S3", subname, nil) |
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 code patch looks good. The addition of the step2CheckDiskSize struct and its Execute() method allows for an additional step in the task. This is helpful in ensuring that the disk is properly formatted and sized as needed before deployment. The code appears to be well structured and written in accordance with the Go programming language conventions.
However, there are a few areas of improvement that could be made. For example, error handling could be improved by adding more descriptive error messages and providing more robust error handling. Additionally, more checks should be added to ensure that the disk size meets the requirements of the application. These measures would help to reduce the risk of bugs or unexpected behavior.
serviceId: serviceId, | ||
storage: curveadm.Storage(), | ||
execOptions: curveadm.ExecOptions(), | ||
}) | ||
|
||
return t, nil | ||
} |
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.
Code Review:
- Code is well indented, which makes it easy to read.
- The code looks complete, with all the necessary variables and functions declared.
- It is good that you have used comments to explain what each function is doing.
- There is no bug risk identified in the code.
- For improvement suggestion, it is better to use constants instead of hardcoded values.
host, dataDir, serviceId); err != nil { | ||
return t, err | ||
} | ||
} | ||
|
||
t.AddStep(&step2GetService{ // if service exist, break task | ||
serviceId: serviceId, |
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.
code review:
- The code seems to be adding functionality to allow the use of disk records in a specific case. However, it is not clear why this functionality is necessary or what it does. It would be helpful to add some comments to explain the purpose of this code.
- The variable 'useDiskRecords' is assigned a boolean value, but it is not clear what the purpose of this variable is. If possible, it should be renamed to something more descriptive.
- The error handling for the call to 'curveadm.Storage().UpdateDiskChunkServerID' is missing. It should include a check for an error and return the appropriate response if one occurs.
} | ||
|
||
return tuicommon.FixedFormat(lines, 2) | ||
} |
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 code review.
- The code is written in proper format and is easy to read.
- The code follows good coding standards and naming conventions like using proper indentation and comments for better readability.
- The code uses a sort.Slice() function for sorting the diskRecords slice which is efficient and optimised.
- The code uses tuicommon for formatting the title and fixed formatting of the lines.
- It would be good to add some unit test cases to ensure the functionality of this code.
for _, item := range s { | ||
func Slice2Map[T comparable](t []T) map[T]bool { | ||
m := map[T]bool{} | ||
for _, item := range t { | ||
m[item] = true | ||
} | ||
return m |
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 code review:
-
The added case for type []interface{} in func Type(v interface{}) is good, it supports dynamic types in a slice which is useful for different situations.
-
The new func IsAnySlice(v interface{}) is good, it provides a boolean result to check if a type is a slice of interface{}.
-
The changed func Slice2Map[T comparable](t []T) is good, it supports generic types which can be used in different situations.
@@ -46,3 +55,6 @@ hosts: | |||
- EXT_PATH=/mnt/memcachefile/cachefile:1024G | |||
- EXT_WBUF_SIZE=8 # size in megabytes of page write buffers. | |||
- EXT_ITEM_AGE=1 # store items idle at least this long (seconds, default: no age limit) | |||
- VERBOSE="v" # v: verbose (print errors/warnings while in event loop) | |||
# vv: very verbose (also print client commands/responses) | |||
# vvv: extremely verbose (internal state transitions) |
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 code review
The code patch looks to be adding a new environment variable called SUDO_ALIAS
to each of the server hosts. Additionally, the VERBOSE
environment variable is set to v
for each of the server hosts.
Overall, it looks like the code patch is valid and does not contain any major bugs. However, it would be beneficial for the maintainability of the code to add some comments to explain why these environment variables are necessary. This will help future developers more easily understand what the code is doing.
It might also be useful to consider if these variables should be defined outside of the server hosts configuration. For example, if the SUDO_ALIAS
variable is always going to have the same value, then it could be defined in a single location. This will help reduce repetition and make the code easier to maintain.
Finally, it is important to ensure that the environment variable values are valid. For example, check that the PORT
variable contains a valid port number and that the LISTEN
variable contains a valid IP address.
precheck | ||
stop_container | ||
rm_cachefile |
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.
code review:
- The patch adds a new function rm_cachefile(), which is used to delete the cache file. This operation must be reliable, so you should consider adding additional validation to check the validity of the cache file path.
- The patch adds the g_rm_cmd variable to store the command for deleting files. It would be better to store this command as a constant instead.
- The patch calls the precheck() function before calling the stop_container() function. This could create potential bugs if any of the variables used in the precheck() function are modified in the stop_container() function. It would be better to call the precheck() function after the stop_container() function.
- The patch calls the rm_cachefile() function at the end of the script. This could cause problems if the precheck() function or stop_container() function fails and the rm_cachefile() function is never called. It would be better to call the rm_cachefile() function at the end of the stop_container() function.
@@ -74,6 +76,9 @@ init() { | |||
if [ "${EXT_ITEM_AGE}" ]; then | |||
g_start_args="${g_start_args} --extended ext_item_age=${EXT_ITEM_AGE}" | |||
fi | |||
if [ "${VERBOSE}" ];then | |||
g_start_args="${g_start_args} -${VERBOSE}" | |||
fi | |||
} | |||
|
|||
create_container() { |
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.
with the code review.
The patch looks good on first glance, as it adds three new variables to the code, namely: g_rm_cmd, g_mkdir_cmd and g_touch_cmd. These should be used to remove, create and update files respectively.
In the precheck() function, there is a check for an EXT_PATH file, which is then removed using the g_rm_cmd variable. However, it could be useful to add an additional check to make sure that the file exists before attempting to remove it, to avoid any errors.
In the init() function, there is a check for an EXT_ITEM_AGE, which is then added to the g_start_args. There is also a check for a VERBOSE paramter, which is then added to the g_start_args. This could be useful for debugging, but it might be good to add a comment to explain why this is being added.
Overall, the code looks good and there don't appear to be any major risks or issues. A few minor improvements could be made, such as the addition of an additional check to make sure the file exists in the precheck() function, and a comment explaining the purpose of the VERBOSE parameter in the init() function.
} | ||
|
||
precheck | ||
start_container |
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 Code review
- There are no comments given in the code
- Hard coded values like g_container_name,g_docker_cmd,g_rm_cmd,g_mkdir_cmd,g_touch_cmd,g_status are used in the code
- No validations are done before executing the commands
- No error handling is done to handle the errors when executing the commands
Suggestions:
- Comments should be given for each code block for better understanding of the code
- The Hard coded values should be passed as parameter for re-usability of the code
- Validations should be done before executing the commands
- Error handling should be done to handle the errors when executing the commands
get_status_container |
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 review:
The code looks correct and there is no bug risk. However, it can be improved by adding extra checks and validations to ensure that the code works as expected. For example, you can add a check to make sure that the container name is valid before attempting to run any of the docker commands, or check if the container is running before calling the show_ip_port() function. Additionally, you can also add logging statements to help track the execution of the code.
No description provided.