User config watcher

[asymmetric]

This PR adds a watcher for the user.toml file.

It's based on @krnowak's still-unmerged work on the file watcher.

[asymmetric]

Wasn't sure how to create a symlink for this. Also, the function is private, so I'm not sure if there's a point.

[krnowak]

I guess this is a note to yourself, because I don't understand what it is about. Not enough context.

[asymmetric]

The svc_file_path function could be rendered as a link, since /// comments are Markdown. I just don't know what the target should be, as it seems to be relative to something.

[asymmetric]

Fixed this.

[asymmetric]

NB: The config is not currently reloaded, so this doesn't work yet.

[krnowak]

Looks good overall, I had some nits.

[krnowak]

If you changed the order of members in Manager struct, it would be good to reorder the initializations here too.

[krnowak]

I guess this is a note to yourself, because I don't understand what it is about. Not enough context.

[krnowak]

This comment does not seem to be true - reset_events_for sets the boolean to false.

[asymmetric]

Yes, this is a note to myself saying that we could change the implementation. True/false values can be represented by presence/absence in a set.

[asymmetric]

Tried using a HashSet and turns out it's more complicated than expected (if we put a HashSet in an Arc, that HashSet is immutable, unless I use mutexes), so not worth it.

[krnowak]

Yeah, you would need to do something similar to what some code in butterfly does - Arc<RwLock<HashSet<Whatever>>>. Anyway, I think that current solution is fine as well.

[krnowak]

Instead of using expect, I'd use outputln! macro to report an error and maybe quit the thread. I'm not sure if we should panic in this case.

[asymmetric]

Good point, I've committed a change.

[krnowak]

Why sleeping instead of wait_for_events?

[krnowak]

Consider adding this log key to the documentation here - www/source/docs/reference/log-keys.html.md.

[krnowak]

Another two things I noticed.

[krnowak]

One note about this - when you use the into_path() function, you also take over the responsibility of removing the created temporary directory. Which you probably don't do, no?

[asymmetric]

Good point, fixed.

[krnowak]

Returning references makes the trait less flexible, because you need an existing, non-temporary thing in memory to return its address (like a member of a struct that implements this trait). In other words - you can't compute the return value. Not sure if it is really important, but in general I would just return plain values. Of course the downside of returning values is that you need to clone the stuff, but OTOH this won't likely be a bottleneck ever.

[asymmetric]

My rationale here is that the name is always a reference to its containing struct.

Isn't it enough for users of this trait to be able to call name.clone() if they want to own the String?

Can you show an example of where the current implementation creates issues? As I'm not sure I understand your comment.

[krnowak]

There are no problems with current implementation. It was just a general note about returning references in trait functions. So in this case it's likely ok, but in general I would avoid it.

[asymmetric]

OK, let me know if you feel strongly about changing this, otherwise I'm gonna leave it as is.

[krnowak]

Nah, leave it as is, please. If somebody else points out this thing, then you can change it.

[asymmetric]

@krnowak addressed your comments, but I guess this will just wait until your PR gets merged.

I also rebased on yours.

I will clean up the commit messages next week.

[krnowak]

I had two questions, otherwise looks good.

[krnowak]

Should this || self.user_config_updated be moved one line below, to the let reconfigure line?

[asymmetric]

No, the needs_reconfiguration flag is only used to trigger a reconfigure hook, which may not be defined. The reload flag is actually going to trigger a service reload, which is what we need here.

[krnowak]

We have an add function. Do we need a remove function then?

[asymmetric]

The PR is in a weird state right now. It shows commit that are not in the PR's branch. What happened is that the base branch got merged into master, and after that happened, I rebased my branch on master.

I then changed the PR to use master as the base branch, but now it shows commits here that shouldn't be in this branch (and aren't there according to git log).
It seems like this is a problem with GitHub's UI.

Also, please don't be too picky about commit messages (and the commits themselves), as I am still testing this PR out.

[asymmetric]

@krnowak could you take a look again?

[krnowak]

Looks mostly fine, but there was one hairy piece of code I'd like to see if it can be "balder".

[krnowak]

To move, not copy. After swapping, the vector under self.services lock will be empty.

[krnowak]

This became a bit hard to follow. Would pseudo-rust below be clearer? (cufr = cfg_updated_from_rumors, sucu = self.user_config_updated, crc = census_ring.changed())

if sucu {
    if let Err(e) = self.cfg.load_user(&self.pkg) {…}
}
let (cfg_changed, reload, reconfigure) = {
    let ctx = …;
    if cufr || crc {
        (cufr || sucu, self.compile_hooks(&ctx) || sucu, self.compile_configuration(&ctx))
    } else if sucu {
        (true, true, self.compile_configuration(&ctx))
    } else {
        (false, self.needs_reload, self.needs_reconfiguration)
    }
}
self.needs_reload = reload;
self.needs_reconfiguration = reconfigure;
self.user_config_updated = false;
/* return */ cfg_changed

[krnowak]

The something_to_do variable would be gone.

[asymmetric]

I find the 3 value tuple even less clear tbh...

(cufr || sucu, self.compile_hooks(&ctx) || sucu, self.compile_configuration(&ctx))

what?

[krnowak]

Not sure what this "what?" is about, but to be clear - I don't actually want to have variables like cufr or sucu. I wrote it like that to avoid typing in the comment.

[asymmetric]

I think that even with more descriptive names, it's hard to parse logically. There's just too much happening on one line: returning a tuple, ORing two values, calling functions..

Not sure it's an improvement.

[krnowak]

You don't need to put everything in one line. You could very well have:

let config_changed = config_updated_from_rumors || self.user_config_updated;
let needs_reload = self.compile_hooks(&ctx) || self.user_config_updated;
let needs_reconfigure = self.compile_configuration(&ctx);
(config_changed, needs_reload, needs_reconfigure)

In general, what I wanted to propose is to simplify the flow of the code, instead of using nested ifs and matches that are sometimes executed and sometimes not.

[asymmetric]

In the above implementation though, we're calling self.compile_hooks even if we don't need to, which will lead to "Hooks recompiled" being printed over and over (this function gets executed on every tick).

I'm definitely open to simplifying the code but obviously it has to have the same behaviour.

The problem is that some of these functions have side effects so we can't just call them willy-nilly.

[krnowak]

The self.compile_hooks(&ctx) should only be executed when either cfg_updated_from_rumors is true or if census_ring.changed() returns true. So if this happens everytime, then something is wrong.

[krnowak]

That's nice.

[krnowak]

I'd add "eventually" - "thereby eventually stopping …"

[asymmetric]

Why eventually? Do you mean that the operation is asynchronous?

If so, I think I can explain it more explicitly.

[krnowak]

thereby is your new favourite word, eh? :)

[asymmetric]

It's a pretty cool word, you gotta admit that

[krnowak]

Drop this empty line?

[asymmetric]

Closing in favor of habitat-sh#3898.