Reload config on update#357
Conversation
|
Thank you for this PR! I'll look it over this weekend. Very excited about this one. Just as a heads up, there's a pending PR to set up go modules in this repo, which might have implications for how you add I'll prioritize merging that PR in so that it unblocks this PR, but you'll probably need to move |
|
Hey @elliotpeele, thanks for being patient with us. #353 has been merged. Do you mind updating this PR to use go modules instead? |
|
I don't mind updating to use modules. BTW, I found an issue with the PR where over time, as the config gets updated, the indexes get removed. I haven't had time to work on a resolution yet though. |
|
Got it. I'll hold off on reviewing this until I hear more, just so I can prioritize clearing out some of the outstanding backlog. |
|
@salemhilal, I finally got back to this. Found why the indexes were getting deleted when the config updated and updated the PR. |
|
Thank you @elliotpeele! I'm a little swamped at the moment, but I haven't forgotten about this PR. I appreciate the update and will give it a closer look shortly. |
|
Hey! Is there any way in which I can help in getting this merged? I know only a little Go and I'm not at all familiar with Hound's codebase, but if it was rebased on top of head I can run this on our test deployment and report any issues! |
|
hey @kkom, I'm going to try and set aside some time to day to validate this PR. Thanks again (to everyone on this PR) for being patient. |
|
Thanks @salemhilal – let me know if any prolonged testing would help, happy to do it, as I'm just about to set up a POC deployment of Hound! |
salemhilal
left a comment
salemhilal
left a comment
There was a problem hiding this comment.
Hey @elliotpeele, I left a bunch of small comments. Let me know what you think, and if you can, rebase the handful of upstream changes into this branch. I'll give it another once-over afterwards (much faster this time, I promise) and I'll merge this in. Thanks again for your contribution.
| RUN apk update \ | ||
| && apk add go git subversion libc-dev mercurial bzr openssh \ | ||
| && apk add go git subversion libc-dev mercurial openssh \ | ||
| && go get github.com/fsnotify/fsnotify \ |
There was a problem hiding this comment.
Do you mind adding this as a go module instead?
| api.Setup(m, idx) | ||
| return http.ListenAndServe(addr, m) | ||
| } | ||
|
|
There was a problem hiding this comment.
These two methods are deleted because they are unused, correct?
| return &foundRefs{ | ||
| refs: refs, | ||
| claimed: map[*index.IndexRef]bool{}, | ||
| claimed: map[string]bool{}, |
There was a problem hiding this comment.
Can you give me some details about why the key type of claimed here was swapped out to be a string rather than a pointer to an IndexRef ?
| if ct != nil { | ||
| // if so, render it | ||
| if err := renderForPrd(w, ct, h.cfg, h.cfgJson, r); err != nil { | ||
| if err := renderForPrd(w, ct, h.cfg, r); err != nil { |
There was a problem hiding this comment.
this diff makes me think that prdHandler.cfgJson should be updated, rather than updating these to all use the reference to the Config object. Otherwise, the two would drift from each other.
|
I'm interested in picking this up if @elliotpeele does not. |
|
@jrarmstro I haven't had time to get back around to this. Feel free to pick it up. |
|
Are there any plans to revive this PR? |
Rep off of hound-search#357 plus removal up of the cvs- directory as well
5 participants
This PR adds automatic reloading of the configuration file when the file is written. It mostly focuses on updates to the repository list. Resolves #287