Allow programmatic configuration of unicast relays.

[mbeards]

This change allows users to configure relays from code without having to setenv(GZ_RELAY).

🎉 New feature

Summary

This change allows users to configure relays from code without having to setenv(GZ_RELAY).

Test it

This can be tested by starting a Node across a boundary where multicast isn't enabled, and setting the relay IP via NodeOptions.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[ahcorde]

include <cstring>

[mbeards]

This file already includes <string.h> - should I swap the include?

[codecov]

Codecov Report

Attention: Patch coverage is 17.64706% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 87.64%. Comparing base (eac2e69) to head (9b823d8).
Report is 16 commits behind head on gz-transport13.

❗ Current head 9b823d8 differs from pull request most recent head 472dde4. Consider uploading reports for the commit 472dde4 to get more accurate results

Files	Patch %	Lines
include/gz/transport/Discovery.hh	0.00%	9 Missing ⚠️
src/NodeOptions.cc	40.00%	3 Missing ⚠️
src/Node.cc	33.33%	2 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           gz-transport13     #497      +/-   ##
==================================================
- Coverage           87.69%   87.64%   -0.05%     
==================================================
  Files                  59       59              
  Lines                5704     5716      +12     
==================================================
+ Hits                 5002     5010       +8     
- Misses                702      706       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[caguero]

My main problem with this patch is that so far NodeOptions are all options that affect a single node. However, adding relays will have a global impact in all the nodes.

I understand that we don't have a way to set global options besides using environment variables right now. Perhaps an alternative could be to not use NodeOptions and just create an AddGlobalRelay() / GlobalRelays() functions in Node to preserve the semantics of NodeOptions? What do you think?

[caguero]

I think you just moved this function. Out of curiosity, any reason for moving it?

[mbeards]

I moved it up to sit with the other public functions. ("Group similar declarations together, placing public parts earlier." https://google.github.io/styleguide/cppguide.html#Declaration_Order)

[caguero]

I'd rename this function to AddRelays() to be more precise. You're not replacing the current relays, just adding extra ones.

[caguero]

We have two options here:

1. Do not create a member variable this->dataPtr->relayIPs when adding the relays. Then, just add the relays using AddRelayAddress() as you're doing it right now. Then, in this function, read the entire list of relays from Discovery.
2. Keep it as it's documenting that the returned vector of relays is the contribution from this node. Other relays could be happening.

I suspect (1) is going to be less confusing and will make debugging easier.

[mbeards]

My main problem with this patch is that so far NodeOptions are all options that affect a single node. However, adding relays will have a global impact in all the nodes.

Agreed that this is weird / gross. I guess ideally I'd want to have accessors for the global Discovery instances, but threading that through seems difficult.

I understand that we don't have a way to set global options besides using environment variables right now. Perhaps an alternative could be to not use NodeOptions and just create an AddGlobalRelay() / GlobalRelays() functions in Node to preserve the semantics of NodeOptions? What do you think?

You're thinking something like the following?

class Node {
  // ...
  public: static void AddGlobalRelay(const std::string&);
  public: static std::vector<std::string>& Relays();
  // ...
}

[caguero]

You're thinking something like the following?

class Node {
  // ...
  public: static void AddGlobalRelay(const std::string&);
  public: static std::vector<std::string>& Relays();
  // ...
}

Probably AddGlobalRelay() and GlobalRelays().

[mbeards]

You're thinking something like the following?

class Node {
  // ...
  public: static void AddGlobalRelay(const std::string&);
  public: static std::vector<std::string>& Relays();
  // ...
}

Probably AddGlobalRelay() and GlobalRelays().

Done in #498