Resolves issue: d015633d10751674d89c9deb6fff2b44feb6a218

No GiHub Actions Workflows found.

I think having a way for the node operator to set a maximum time for a CI run makes sense, and that the CI broker should gain that feature. However, I have notes on this implementation.

• cargo test fails for me. I did not look into why.

• It runs the adapter using the timeout command. This is apparently not available on macOS by default. So far, the CI broker has worked on Macs, so this would a regression. I like the simplicity of using timeout, but I want the CI broker to work on Macs. I’m not sure that is a hard requirement by the Radicle project, though. Could you check with, say, cloudhead?

• You made the CI broker configuration to require a timeout_minutes field. This breaks every existing installation. I’d want the field to be optional, with some sensible default, such as 10 minutes.

• I don’t like the name of the field. timeout should be enough, and it should ideally accept a unit suffix: 1h vs 60min, but default minutes if there is no suffix.

• In fact, possibly the time out should be configurable for each repository: different projects may need more time or always need less time. That’s going to require big changes to the CI broker configuration file, though, which will interact badly with other changes I want to do. Thus, I’m just noting this here, your patch should not add this.

• You rename a local variable x in src/adapter.rs. While the change may be OK, it’s irrelevant to the patch. Please keep a patch focused.

Thanks for the feedback! I have pushed a new revision addressing these things:

• cargo test now passes tests successfully - I am not sure how I missed that out. I am not sure if the approach I followed is the recommended one here, so, feedback is more than welcome!
• Even through we might don’t want to support MacOS officially, lots of us probably run the broker in MacOS. So, I updated the code to work for both OSs.
• timeout_minutes was optional and worked even without that being present at the config file. Now renamed to timeout and its an optional string.
• Added support of s, m, h as prefixes to timeout and parsed through parse_duration crate. Default timeout set to 1h as 10 mins might limit some existing adapters’ execution
• This makes completely sense for me as well. Having said that I would also like to have a default limit per adapter. Some adapter generally require less time (e.g. webhooks) compared to others (e.g. CI).
• Reverted this back. Will open another patch to address this!

Some notes for the latest revision:

• new dependency parse_duration is an unmaintained crate: no updates in three years, the README starts with “IMPORTANT: This repository is no longer being updated. Before deciding to use it, check if any of the issues are deal breakers. In particular, this crate should not be used with untrusted input (see this issue).”
  • I’d really rather not add this dependency
  • is there another crate that would be maintained that we can use instead?
  • if not, we can make do with a custom parser for a few supported time units: an integer followed by nothing (meaning seconds), “s”, “m”, “h”
• radicle::node::DEFAULT_TIMEOUT is described as “timeout when waiting for the node to respond with data” – why is it a good choice for the default timeout for a CI run?
• is gtimeout part of the macOS installation? are we sure timeout is available on other, non-Linux platforms, such as BSD?
• the constants 126 and 127 should be named so that they aren’t so mysterious
• once the code is ready, I’ll want to add acceptance criteria for the timeout feature: I can do that myself, or I can teach you do that, as you prefer

Actually, I’m now thinking that using an external program (“timeout”) may end up being more of a headache than implementing the logic in src/adapter.rs directly. This needs thinking.

On the one hand, timeout implements the logic to deal with signals etc. On the other hand, it’s not universally portable. While Radicle itself isn’t universally portable, it makes sense to me to avoid making things worse.

I’ve implemented this in a different way myself, without using an external command.