-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add search index to generated configmap #205
Add search index to generated configmap #205
Conversation
d639480
to
18567fc
Compare
18567fc
to
69441f7
Compare
69441f7
to
43f5a52
Compare
2ab74e1
to
5af6346
Compare
5af6346
to
4902a56
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.
Overall I think this looks good - just one or 2 small comments.
4902a56
to
3636b39
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.
I'm OK merging it as is, but I offered a few nitpicks
controllers/reconcile.go
Outdated
Title: searchEntry.Title, | ||
Description: searchEntry.Description, | ||
Href: searchEntry.Href, | ||
AltTitle: searchEntry.AltTitle, |
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.
AltTitle
is a string slice, so its a reference. So you may want to do a proper deep copy here like
// Create a deep copy of AltTitle slice
altTitleCopy := make([]string, len(searchEntry.AltTitle))
copy(altTitleCopy, searchEntry.AltTitle)
|
||
func setupSearchIndex(feList *crd.FrontendList) []crd.SearchEntry { | ||
searchIndex := []crd.SearchEntry{} | ||
for _, frontend := range feList.Items { |
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.
Over the course of this change you switch between felist
and feList
- probably better to just go with one.
*groupTiles = append(*groupTiles, *tile) | ||
} else { | ||
// ignore the tile if destination does not exist | ||
skippedTiles = append(skippedTiles, tile.ID) |
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.
Would this be something you want to log?
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.
Yeah, we do want to know in case we have some reports of missing elements.
group := crd.FrontendServiceCategoryGroupGenerated{ | ||
ID: gr.ID, | ||
Title: gr.Title, | ||
Tiles: &tiles, |
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 think you can remove the pointer here because slices are already reference types. You might be able to do just Titles: titles
but I'm not 100% sure. Might be fun to find out!
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 think I already figured it out. And I can't I need the reference later and without the pointer the slice was not updated and the output was always an empty array.
@@ -853,6 +853,97 @@ func setupFedModules(feEnv *crd.FrontendEnvironment, frontendList *crd.FrontendL | |||
return nil | |||
} | |||
|
|||
func adjustSearchEntry(searchEntry *crd.SearchEntry, frontend crd.Frontend) crd.SearchEntry { | |||
newSearchEntry := crd.SearchEntry{ |
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.
You might want to guard on a nil check on searchEntry
? I know you are only calling this in a loop that is already guarded on a nil check, but if this ever ended up getting called somewhere else someday could result in a panic.
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'll do the check in the iteration, the value must exist.
@adamrdrew I'll address them 🙂 |
https://issues.redhat.com/browse/RHCLOUD-35426
Changes