Skip to content

Conversation

@kixelated
Copy link
Collaborator

@kixelated kixelated commented Oct 25, 2025

Summary by CodeRabbit

  • Chores
    • Cluster configuration key renamed from "advertise" to "node" across config, CLI flag, and environment variable.
    • Backward-compatibility aliases preserved for existing deployments during migration.
    • End-user behavior (connectivity announcements/origin filtering) remains unchanged aside from the new key name.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 25, 2025

Walkthrough

The pull request renames the cluster address key from advertise to node across the codebase. Changes: TOML config rs/moq-relay/cfg/leaf.toml renamed the key; rs/moq-relay/src/cluster.rs replaces ClusterConfig.advertise with ClusterConfig.node and adds a serde(alias = "advertise") plus a new --cluster-node clap flag (with backward alias); rs/nix/modules/moq-relay.nix renames MOQ_CLUSTER_ADVERTISE to MOQ_CLUSTER_NODE.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "rename --cluster-advertise back to --cluster-node" is directly related to the main changes in the changeset. The PR systematically renames the cluster configuration from "advertise" to "node" across multiple components: the TOML config key, the ClusterConfig struct field, the environment variable, and the command-line argument. The title specifically highlights the CLI argument change, which is a key part of the public API surface. The title is clear, specific (mentioning both old and new names), and avoids vague terminology. A developer scanning the commit history would easily understand that the cluster naming scheme is being changed from "advertise" to "node."
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cluster-node

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
rs/moq-relay/src/cluster.rs (2)

24-28: CLI compat looks good; confirm env var deprecation and prefer long_alias.

  • serde alias keeps config files with advertise working. Good.
  • CLI alias --cluster-advertise works, but consider using long_alias = "cluster-advertise" for clarity. Functionally equivalent, more explicit.
  • The env var changed to MOQ_CLUSTER_NODE with no alias for MOQ_CLUSTER_ADVERTISE. If dropping the old env var is intentional, document it in release notes. If not, add a lightweight fallback (e.g., hydrate node from std::env::var("MOQ_CLUSTER_ADVERTISE") when unset) in Cluster::new.

Also add colocated tests to lock this in (per guidelines):

  • clap: both --cluster-node and --cluster-advertise populate node.
  • serde: TOML with node = "x" and with advertise = "y" both deserialize.

Example tests to add within this file:

#[cfg(test)]
mod tests {
    use super::*;
    use clap::Parser as _;

    #[derive(clap::Parser, Debug)]
    struct TestCli {
        #[command(flatten)]
        cluster: ClusterConfig,
    }

    #[test]
    fn cli_accepts_node_and_advertise_alias() {
        let p = TestCli::parse_from(["bin", "--cluster-node", "a.example:443"]);
        assert_eq!(p.cluster.node.as_deref(), Some("a.example:443"));

        let p = TestCli::parse_from(["bin", "--cluster-advertise", "b.example:443"]);
        assert_eq!(p.cluster.node.as_deref(), Some("b.example:443"));
    }

    #[test]
    fn serde_accepts_node_and_advertise() {
        let cfg: ClusterConfig = toml::from_str(r#"node = "n.example:443""#).unwrap();
        assert_eq!(cfg.node.as_deref(), Some("n.example:443"));

        let cfg: ClusterConfig = toml::from_str(r#"advertise = "a.example:443""#).unwrap();
        assert_eq!(cfg.node.as_deref(), Some("a.example:443"));
    }
}

109-109: Self-connect guard is correct; consider normalization to avoid false mismatches.

Direct string equality can misfire if casing or formatting differs (e.g., EXAMPLE.com:443 vs example.com:443). Consider normalizing to lower-case host and explicit port before comparing, or parse via Url and compare host+port tuples. Optional but improves robustness.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e169ebc and 5180382.

📒 Files selected for processing (1)
  • rs/moq-relay/src/cluster.rs (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
rs/moq-relay/**

📄 CodeRabbit inference engine (CLAUDE.md)

In the CDN/relay (moq-relay), do not include application logic, media codecs, or track-specific details; keep it transport-agnostic.

Files:

  • rs/moq-relay/src/cluster.rs
rs/**/src/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

Write Rust tests integrated within source files (unit tests alongside code).

Files:

  • rs/moq-relay/src/cluster.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Check
🔇 Additional comments (2)
rs/moq-relay/src/cluster.rs (2)

121-127: Leaf announcement gating on node is appropriate.

Only advertise when we’re connecting to a root and have a node value. The log context includes prefix and myself, which is useful for tracing.


189-190: Correct self-skip in discovery.

The Option<&str> comparison against self.config.node.as_deref() cleanly filters our own origin. LGTM.

@kixelated kixelated merged commit b552c2c into main Oct 25, 2025
1 check passed
@kixelated kixelated deleted the cluster-node branch October 25, 2025 17:49
@moq-bot moq-bot bot mentioned this pull request Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants