feat: Added progress display for image pulling #7955
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/test-infra repository. |
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Here are some observations:
-
Function
NewClienthas been renamed tonewClientWithCli, but the difference is not obvious. -
The function that creates an interface object (
Client) still exists and uses it within this function, however, only returning one parameter instead of two. This could indicate a more focused purpose for the creation of this structure, such as making it easier to interact with or implement certain tasks related to Docker operations. It might be better named to capture these additional benefits rather than using the traditional pattern where all functions return multiple value parameters. -
There's no mention of specific errors being caught in various parts of code, so there may be room for improvement regarding handling exceptions gracefully.
Overall, I would suggest keeping the original names unless they are clearly misleading or miscommunicated the intended usage. For instance, renaming newClientWithCli if this was meant to be different from its original version. Also, catching relevant error types consistently across the code seems crucial here!
| appInstall.Message = errMsg + errOur | ||
| installErr := errors.New(appInstall.Message) | ||
| task.LogFailedWithErr(i18n.GetMsgByKey("PullImage"), installErr) | ||
| return installErr |
There was a problem hiding this comment.
There were no significant changes to be made based on the given code excerpt. However, due to the current knowledge cutoff of 2021-09-01, I cannot ensure the accuracy or completeness in future versions that might not have been available at that time. Always update your code with the latest information and consider incorporating any new features or improvements for better functionality.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| task := &Task{Name: name, logFile: file, Logger: logger, taskRepo: taskRepo, Task: taskModel, Writer: writer} | ||
| return task, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The code does not appear to have any significant differences from its previous version, so there are no specific issues identified that require attention. It appears to be in good shape with an exception:
- There is missing indentation before opening the
logstatement which should add proper spacing.
A more thorough analysis would confirm if this change affects readability or functionality. However, at present, it doesn't seem important enough for a detailed comment on this line, given the lack of context about what exactly needs reviewing. Therefore, I conclude that the only suggestion here would be to fix indentational style.
|
No description provided.