-
Notifications
You must be signed in to change notification settings - Fork 988
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
Expose last warnings from eventlog for Pod+PVC+SVC #16
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.
otherwise LGTM :)
} | ||
return fmt.Errorf("%s%s", err, reason) | ||
return fmt.Errorf("%s%s", err, warns) |
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.
given it's being used in multiple resources, any reason to not pull this out into a separate method?
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.
As discussed, we can do this later if the error handling sprawls through more resources. I think there's still benefit in not decoupling it as we're making an API call here and it should be more obvious from these extra LOC that something "heavy-weighted" is happening here. 🙂
return wErr | ||
} | ||
if len(lastWarnings) > 0 { | ||
warns = fmt.Sprintf(". Last warnings: %s", stringifyEvents(lastWarnings)) |
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.
I'd argue this would read cleaner if only returning the combined err + warns object within this if statement?
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.
Sure, I'll change that. 👍
5e0aa8b
to
4aaf4a0
Compare
Fix Ingress update error:
Examples
Service
PVC
Pod