-
Notifications
You must be signed in to change notification settings - Fork 525
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
[Nuctl] Change error handling logic for function import #3106
Conversation
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.
Minor English suggestions, otherwise lgtm
Co-authored-by: TomerShor <[email protected]>
pkg/nuctl/command/import.go
Outdated
if importFailed.Load() { | ||
return errors.New("Import failed for some of the functions") | ||
} |
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.
this needs to be more indicative. at least provide function names
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.
@liranbg then we will logs them twice - firstly right from goroutine and secondly from here
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.
@rokatyy It's not that bad to log that again, because with many functions the logs can be noisy and it can be easy to miss warnings/errors.
a single log line at the end can be very useful.
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.
@TomerShor Agreed. I've added TODO for this. The plan is to create a custom structure importReport
in the following PR which will contain methods like Print
and SaveToFile
.
pkg/nuctl/command/import.go
Outdated
if importFailed.Load() { | ||
return errors.New("Import failed for some of the functions") | ||
} |
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.
@rokatyy It's not that bad to log that again, because with many functions the logs can be noisy and it can be easy to miss warnings/errors.
a single log line at the end can be very useful.
Jira - https://jira.iguazeng.com/browse/NUC-118
It's just a first iteration of upcoming changes in nuctl import logic.
In this PR, we introduce error logging for each function import process.
We are moving away from using errgroup to enable the tracking of errors for each import individually. This change will enable us to generate a finely-grained report (in following PR) for each failed import, complete with a detailed description and recommendations.