-
Notifications
You must be signed in to change notification settings - Fork 60
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
Use go-functional
in repositories list
methods
#3383
Conversation
dcbaa3c
to
0f99344
Compare
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.
Using go-functional v2, see - ignore I meant to add this as an inline comment.it.Lines
- you can iterate over lines from a reader.
func readLines(r io.Reader, logger logr.Logger) []string { | ||
lines := []string{} | ||
|
||
var err error | ||
var line []byte | ||
bufReader := bufio.NewReader(r) | ||
for { | ||
line, err = bufReader.ReadBytes('\n') | ||
lines = append(lines, string(line)) | ||
if err != nil { | ||
if err != io.EOF { | ||
logger.Info("failed to parse pod logs", "err", err) | ||
} | ||
break | ||
} |
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.
Using go-functional v2, see it.Lines
- you can iterate over lines from a reader.
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.
lines, errs := itx.LinesString(r).Unzip()
if len(errs.Collect()) > 0 {
panic("Ahhh")
}
lines.Collect()
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.
Or if you know reading cannot fail such as with reading from a string buffer:
lines := itx.LinesString(r).Left().Collect()
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.
We wanted to get as much logs as we can regardless read errors. go-functional v1 LinesString did not look it supported that (i.e. it just failed) so we kept that code. When switching to v2 we would make use of itx.LinesString
for sure
api/repositories/pod_repository.go
Outdated
logReadCloser, err := k8sClient.CoreV1().Pods(pod.Namespace).GetLogs(pod.Name, &corev1.PodLogOptions{Timestamps: true, TailLines: &limit}).Stream(ctx) | ||
if err != nil { | ||
logger.Info("failed to fetch logs", "pod", pod.Name, "reason", err) | ||
return iter.Lift([]LogRecord{}) |
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.
For this there's the more explicit iter.Exhausted[LogRecord]()
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.
ah, nice! Will use that :)
fd4eeed
to
42b9d55
Compare
This eliminates quite some boilerplate code around filtering and mapping k8s resources. `go-functional` also replaces our inhouse filtering implementation. Also, this change removes code duplications in methods such as `GetSomethingByAandB` that list and filter resources. Instead of implementing the list operation themselves, they delegate to the relevant `list` method by constructing a list messages that matches the criteria they need. As a further improvement we could get rid of those methods and just invoke `repo.List` from the handlers. As a consequence of this change those methods now return a `NotFound` error rather than `Unauthorised` when the user is not allowed to list the resources. However, that is fine as the related api handlers mask fobidden errors as not found, so no change from API users' point of view Co-authored-by: Danail Branekov <danailster@gmail.com> Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>
42b9d55
to
9ca1d22
Compare
Is there a related GitHub Issue?
No
What is this change about?
This eliminates quite some boilerplate code around filtering and mapping
k8s resources.
go-functional
also replaces our inhouse filteringimplementation.
Also, this change removes code duplications in methods such as
GetSomethingByAandB
that list and filter resources. Instead ofimplementing the list operation themselves, they delegate to the
relevant
list
method by constructing a list messages that matches thecriteria they need. As a further improvement we could get rid of those
methods and just invoke
repo.List
from the handlers.As a consequence of this change those methods now return a
NotFound
error rather than
Unauthorised
when the user is not allowed to listthe resources. Still, that should be OK
Does this PR introduce a breaking change?
No
Tag your pair, your PM, and/or team
@georgethebeatle