feat: Add Netplan configuration validator #668
Conversation
Signed-off-by: karan-palan <karanpalan007@gmail.com>
Karan-Palan
requested review from
Anshgrover23,
Suyashd999 and
mikejmorgan-ai
as code owners
CLA Verification PassedAll contributors have signed the CLA.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR adds a comprehensive Netplan configuration validator that provides YAML syntax validation, semantic checks for IPs and routes, diff generation, dry-run capabilities with auto-revert, backup management, and connectivity testing. It includes CLI integration, extensive test coverage, and user documentation. Changes
Sequence DiagramssequenceDiagram
actor User
participant CLI as CLI Handler
participant Validator as NetplanValidator
participant FS as Filesystem
participant Network as Network Interface
User->>CLI: cortex netplan validate config.yaml
CLI->>Validator: validate_file(config.yaml)
Validator->>FS: Read YAML file
FS-->>Validator: File content
Validator->>Validator: Parse YAML syntax
Validator->>Validator: Validate IPs/routes/gateways
Validator->>Validator: Collect errors/warnings
Validator-->>CLI: ValidationResult
CLI->>CLI: Print results (errors/warnings/info)
CLI-->>User: Exit code 0 (valid) or 1 (invalid)
sequenceDiagram
actor User
participant CLI as CLI Handler
participant Validator as NetplanValidator
participant FS as Filesystem
participant Network as Network Interface
User->>CLI: cortex netplan dry-run config.yaml --new-config new.yaml
CLI->>Validator: dry_run_with_revert(new.yaml, timeout=60)
Validator->>FS: Backup current config
FS-->>Validator: Backup created
Validator->>FS: Apply new config
FS-->>Validator: Applied
Validator->>Network: Ping 8.8.8.8, 1.1.1.1
Network-->>Validator: Connectivity OK
Validator->>User: Show diff, countdown (60s)
User->>Validator: Confirm/reject
alt User confirms
Validator->>CLI: Return success
CLI-->>User: Exit code 0
else User rejects or timeout
Validator->>FS: Restore backup
FS-->>Validator: Restored
Validator->>CLI: Return failure
CLI-->>User: Exit code 1
end
sequenceDiagram
actor User
participant CLI as CLI Handler
participant Validator as NetplanValidator
participant FS as Filesystem
User->>CLI: cortex netplan apply --new-config new.yaml
CLI->>Validator: show_diff(new.yaml)
Validator->>FS: Read current & new configs
FS-->>Validator: File contents
Validator->>Validator: Generate unified diff
Validator-->>CLI: Diff output
CLI->>User: Display diff + confirmation prompt
User->>CLI: Confirm apply
CLI->>Validator: apply_config(new.yaml)
Validator->>FS: Backup current config
FS-->>Validator: Backup saved
Validator->>FS: Apply new config via netplan apply
FS-->>Validator: Applied (0) or Error (1)
alt Apply success
Validator-->>CLI: Return success
CLI-->>User: Exit code 0
else Apply failure
Validator->>FS: Restore backup
FS-->>Validator: Restored
Validator-->>CLI: Return failure with remediation guidance
CLI-->>User: Exit code 1
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Summary of ChangesHello @Karan-Palan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a Netplan configuration validator to the Cortex CLI, enhancing network configuration management by providing features such as YAML syntax and semantic validation, configuration diff previews, and a dry-run mode with auto-revert. The goal is to prevent network outages caused by configuration errors and provide a safer, more user-friendly experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a comprehensive Netplan configuration validator, which is a valuable addition for preventing network outages due to configuration errors. The implementation includes robust features like YAML syntax and semantic validation, configuration diff previews, dry-run mode with auto-revert, and automatic backups. The new netplan_validator.py module is well-structured, and the integration into cortex/cli.py is clean. The accompanying documentation and example script are clear and helpful. The test suite is extensive, covering many scenarios and achieving good coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@cortex/netplan_validator.py`:
- Around line 472-503: The code copies the new config to self.config_file before
running netplan apply so if apply fails the broken file remains; change the flow
so when backup is requested call self.backup_current_config() and capture/retain
the backup path (or have backup_current_config return it), then after
shutil.copy2(new_path, self.config_file) if subprocess.run(["netplan","apply"])
returns non‑zero or raises TimeoutExpired/Exception, restore the original config
by copying the saved backup path back to self.config_file (use shutil.copy2) and
include any restoration error details in the returned error string; reference
validate_file, backup_current_config, shutil.copy2, subprocess.run, and
self.config_file when making the fix.
- Around line 220-247: The validator currently assumes 'network' and its
sections are mappings and will crash if they are other types; update the checks
so after retrieving network = config["network"] you validate isinstance(network,
dict) and return a ValidationResult error if not, and similarly guard each
section before iterating: for "ethernets", "wifis", and "bridges" confirm
network.get(section) is a dict (append a clear error like "section must be a
mapping") and only call .items() and _validate_interface(...) when it is a dict;
reference ValidationResult, network, ethernets, wifis, bridges, and
_validate_interface in your changes.
- Around line 96-114: The netplan autodetection in _find_netplan_config only
looks for "*.yaml" so it misses files using the ".yml" extension; update
_find_netplan_config to include both "*.yaml" and "*.yml" (e.g., gather files
from self.NETPLAN_DIR.glob("*.yaml") and self.NETPLAN_DIR.glob("*.yml") or a
single pattern that matches both, combine and sort the results) and return the
first sorted Path as currently done, keeping the same FileNotFoundError behavior
when the combined list is empty.
- Around line 546-551: The code calls self.apply_config(new_config_file,
backup=False) and on failure returns without reverting; for dry-run flows ensure
a backup is created and restore it on apply failure: when performing a dry-run
call apply_config with backup=True (so a backup path is available), and if
success is False then call the restore path (e.g.,
self.apply_config(backup_path) or existing restore_backup/ revert_config helper)
to re-apply the prior config and remove the transient new_config_file; reference
apply_config, new_config_file and the backup flag in the change.
In `@docs/NETPLAN_VALIDATOR_IMPLEMENTATION.md`:
- Around line 20-23: The documentation states the test coverage for
tests/test_netplan_validator.py is 67% which is below the project-required >80%;
either add tests to raise coverage above 80% (focus on uncovered functions in
the netplan validator test suite) or update the documented numbers and threshold
in NETPLAN_VALIDATOR_IMPLEMENTATION.md (replace "67%" and the "55% requirement"
text with the new coverage percent and the correct >80% requirement), and make
the same change where the file references lines 258-259 to keep the doc
consistent; reference tests/test_netplan_validator.py and the coverage
percentage text when making edits.
- Around line 88-112: Update the fenced code blocks in
NETPLAN_VALIDATOR_IMPLEMENTATION.md that show the coverage table, test results,
and code-style output to include explicit language identifiers (e.g., change ```
to ```text) so markdownlint stops flagging them; specifically edit the fenced
blocks containing the coverage table, the test results block, and the ruff check
output (the second similar fenced block further down) to use ```text.
In `@docs/NETPLAN_VALIDATOR.md`:
- Around line 351-355: Add a language identifier to the fenced code block that
shows the backup listing (the block starting with ``` and containing
"~/.cortex/netplan_backups/"), e.g., change the opening fence to ```text (or
```bash) so markdownlint passes; update the block that lists
"01-netcfg_20260117_143022.yaml" and "01-netcfg_20260117_151530.yaml"
accordingly.
- Around line 284-292: Replace the YAML "Bad (tabs)" example that contains a
literal tab before the "version: 2" line (the lines showing "network:" and
"version: 2") with a version that does not include a hard tab—either remove that
example entirely or show the tab as an explicit visible marker (e.g., "\t" or
"␉") so the markdown no longer contains a literal tab character; keep the "Good
(spaces)" example as-is to demonstrate the correct indentation.
In `@tests/test_netplan_validator.py`:
- Around line 21-96: Add missing type hints to all fixtures: annotate parameters
and return types for temp_netplan_dir(tmp_path: pathlib.Path) -> pathlib.Path,
temp_backup_dir(tmp_path: pathlib.Path) -> pathlib.Path, valid_netplan_config()
-> str, invalid_yaml_config() -> str, invalid_ip_config() -> str, and
netplan_validator(temp_netplan_dir: pathlib.Path, temp_backup_dir: pathlib.Path,
valid_netplan_config: str, monkeypatch: pytest.MonkeyPatch) -> NetplanValidator;
import any needed typing (pathlib.Path, pytest.MonkeyPatch) and ensure the
NetplanValidator return type is referenced or imported so the annotations are
valid.
- Around line 133-191: The test test_cidr_when_not_allowed incorrectly expects
"192.168.1.1/24" to be valid when calling validate_ip_address(...,
allow_cidr=False); change the test in
TestIPAddressValidation.test_cidr_when_not_allowed to assert that valid is False
and that the returned error mentions CIDR/not allowed (e.g., assert valid is
False and "cidr" or "not allowed" in error.lower()), or alternatively
rename/clarify the allow_cidr flag if lenient behavior is intended; locate
validate_ip_address and test_cidr_when_not_allowed when making the change.
🧹 Nitpick comments (2)
cortex/netplan_validator.py (1)
45-65: Preferdefault_factoryfor collection fields inNetworkInterface.
addresses,nameservers, androutesare typed as collections but default toNone. Usefield(default_factory=...)to avoid None checks and align with dataclass idioms.♻️ Proposed refactor
-from dataclasses import dataclass +from dataclasses import dataclass, field ... `@dataclass` class NetworkInterface: """Represents a network interface configuration.""" name: str dhcp4: bool = False dhcp6: bool = False - addresses: list[str] = None + addresses: list[str] = field(default_factory=list) gateway4: str | None = None gateway6: str | None = None - nameservers: dict[str, Any] | None = None - routes: list[dict[str, str]] | None = None - - def __post_init__(self): - """Initialize default values.""" - if self.addresses is None: - self.addresses = [] - if self.nameservers is None: - self.nameservers = {} - if self.routes is None: - self.routes = [] + nameservers: dict[str, Any] = field(default_factory=dict) + routes: list[dict[str, str]] = field(default_factory=list)tests/test_netplan_validator.py (1)
398-432: Avoid real sleeps in tests for backup uniqueness.
time.sleep(1.1)slows the suite and can still be flaky under clock jitter. Consider mocking the timestamp source inbackup_current_config(e.g., patch time/datetime) to generate deterministic unique names without sleeping.
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive Netplan configuration validator to prevent network outages from simple YAML typos, addressing GitHub issue #445. The implementation includes YAML syntax validation, semantic validation for IP addresses/routes/gateways, configuration diff previews, dry-run mode with automatic revert, and automatic backups.
Changes:
- Added
cortex/netplan_validator.py(799 lines) with core validation logic, diff generation, dry-run with auto-revert, and network connectivity testing - Added comprehensive test suite
tests/test_netplan_validator.py(1364 lines, 82 test methods) - Integrated validator into CLI with four commands: validate, diff, apply, and dry-run
- Added documentation and usage examples
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/netplan_validator.py | Core implementation with validation logic, safety mechanisms (backups, auto-revert), and network connectivity testing |
| tests/test_netplan_validator.py | Comprehensive test suite covering YAML syntax, IP validation, routes, semantics, file operations, backups, and edge cases |
| cortex/cli.py | CLI integration adding cortex netplan command with four subcommands and appropriate error handling |
| examples/test_netplan_validator.sh | Demo script with example configurations for testing all validator features |
| docs/NETPLAN_VALIDATOR.md | User documentation with usage examples, configuration formats, validation rules, and troubleshooting |
| docs/NETPLAN_VALIDATOR_IMPLEMENTATION.md | Implementation summary with architecture details and metrics (contains outdated statistics) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Karan-Palan Address all bot comments.
|
On it! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/netplan_validator.py`:
- Around line 253-330: The _validate_interface method currently assumes
iface_config is a dict and will crash if it's not; add an early type check
(e.g., isinstance(iface_config, dict) or isinstance(iface_config, Mapping)) at
the top of _validate_interface to detect non-mapping configs, append a clear
error to the errors list like "Interface '<name>' config must be a
mapping/dictionary", and return immediately to avoid further .get() or
iteration; ensure all subsequent uses still reference iface_config (no other
refactor required).
♻️ Duplicate comments (5)
cortex/netplan_validator.py (5)
96-114: Include.ymlconfigs in auto-detection.Auto-detection misses common
.ymlNetplan files, which can cause validation to target the wrong file or fail outright.🐛 Suggested fix
- yaml_files = list(self.NETPLAN_DIR.glob("*.yaml")) + yaml_files = sorted(self.NETPLAN_DIR.glob("*.yaml")) + sorted( + self.NETPLAN_DIR.glob("*.yml") + ) if not yaml_files: raise FileNotFoundError(f"No .yaml files found in {self.NETPLAN_DIR}") # Use the first yaml file (typically 00-installer-config.yaml or 01-netcfg.yaml) - return sorted(yaml_files)[0] + return sorted(yaml_files)[0]
152-170: Reject CIDR input whenallow_cidr=False.As written,
"192.168.1.1/24"is accepted even when CIDR is disallowed, so invalid gateways/nameservers can pass validation.🐛 Suggested fix
- try: - if allow_cidr and "/" in ip_str: - # Validate CIDR notation - ipaddress.ip_network(ip_str, strict=False) - else: - # Validate plain IP - ipaddress.ip_address(ip_str.split("/")[0] if "/" in ip_str else ip_str) + try: + if "/" in ip_str: + if not allow_cidr: + return False, f"CIDR notation is not allowed: '{ip_str}'" + # Validate CIDR notation + ipaddress.ip_network(ip_str, strict=False) + else: + # Validate plain IP + ipaddress.ip_address(ip_str) return True, ""
220-247: Guard against non-mappingnetwork/section values to avoid crashes.If
networkor its sections are lists/strings,.items()will raise and the validator crashes instead of returning actionable errors.🐛 Suggested fix
- network = config["network"] + network = config["network"] + if not isinstance(network, dict): + errors.append("'network' must be a mapping/object") + return ValidationResult(False, errors, warnings, info) ... - if "ethernets" in network: - for iface_name, iface_config in network["ethernets"].items(): - self._validate_interface(iface_name, iface_config, errors, warnings, info) + if "ethernets" in network: + if not isinstance(network["ethernets"], dict): + errors.append("'network.ethernets' must be a mapping/object") + else: + for iface_name, iface_config in network["ethernets"].items(): + self._validate_interface(iface_name, iface_config, errors, warnings, info)Apply the same guard pattern to
wifisandbridges.
456-503: Revert to backup ifnetplan applyfails after copy.If
netplan applyfails or times out, the new (possibly broken) config remains on disk, defeating the safety guarantees.🐛 Suggested fix
- if backup: - try: - self.backup_current_config() - except Exception as e: - return False, f"Backup failed: {str(e)}" + backup_path: Path | None = None + if backup: + try: + backup_path = self.backup_current_config() + except Exception as e: + return False, f"Backup failed: {str(e)}" @@ - if result.returncode == 0: - return True, "Configuration applied successfully" - else: - return False, f"netplan apply failed: {result.stderr}" + if result.returncode == 0: + return True, "Configuration applied successfully" + if backup_path: + self._revert_config(backup_path) + return False, f"netplan apply failed: {result.stderr}" except subprocess.TimeoutExpired: - return False, "netplan apply timed out" + if backup_path: + self._revert_config(backup_path) + return False, "netplan apply timed out" except Exception as e: - return False, f"Failed to apply config: {str(e)}" + if backup_path: + self._revert_config(backup_path) + return False, f"Failed to apply config: {str(e)}"
505-551: Dry-run should revert if apply fails.On apply failure, dry-run returns without restoring the previous config, leaving the system in an unsafe state.
🐛 Suggested fix
success, message = self.apply_config(new_config_file, backup=False) if not success: console.print(f"\n[bold red]Apply failed:[/bold red] {message}") + self._revert_config(backup_path) return False
Signed-off-by: karan-palan <karanpalan007@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_netplan_validator.py`:
- Around line 1-1351: Update the coverage thresholds to enforce >80%: set the
coverage "fail_under" value in pyproject.toml to 81 (or another integer >80) and
update the CI workflow invocation in .github/workflows/ci.yml to pass
--cov-fail-under=81 (and ensure the coverage collection includes the Python file
pattern **/*.py); change both locations so they match and enforce the same >80%
threshold.
♻️ Duplicate comments (1)
tests/test_netplan_validator.py (1)
21-96: Add type hints to fixtures to comply with project guidelines.The fixtures and test functions lack type annotations. Per coding guidelines, type hints are required in Python code.
♻️ Example pattern for fixtures
`@pytest.fixture` -def temp_netplan_dir(tmp_path): +def temp_netplan_dir(tmp_path: Path) -> Path: """Create a temporary netplan directory structure.""" netplan_dir = tmp_path / "etc" / "netplan" netplan_dir.mkdir(parents=True) return netplan_dir `@pytest.fixture` -def temp_backup_dir(tmp_path): +def temp_backup_dir(tmp_path: Path) -> Path: """Create a temporary backup directory.""" backup_dir = tmp_path / "backups" backup_dir.mkdir(parents=True) return backup_dir `@pytest.fixture` -def valid_netplan_config(): +def valid_netplan_config() -> str: """Return a valid netplan configuration.""" # ... `@pytest.fixture` -def netplan_validator(temp_netplan_dir, temp_backup_dir, valid_netplan_config, monkeypatch): +def netplan_validator( + temp_netplan_dir: Path, + temp_backup_dir: Path, + valid_netplan_config: str, + monkeypatch: pytest.MonkeyPatch, +) -> NetplanValidator: """Create a NetplanValidator instance with mocked paths.""" # ...As per coding guidelines, type hints are required.
🧹 Nitpick comments (1)
tests/test_netplan_validator.py (1)
1040-1043: Remove redundantimport yamlstatements inside test methods.The
yamlmodule is already imported at line 12. These local imports inside test methods are unnecessary.♻️ Suggested fix
def test_version_not_equal_to_2(self, temp_netplan_dir): """Test warning when version is not 2.""" # ... config setup ... validator = NetplanValidator(str(config_file)) # Load and parse the config - import yaml - with open(config_file) as f: config_dict = yaml.safe_load(f)Apply the same change to:
test_invalid_gateway6(line 1071)test_invalid_nameserver_addresses(line 1101)test_valid_nameservers(line 1131)test_interface_with_invalid_routes(line 1165)Also applies to: 1071-1074, 1101-1104, 1131-1134, 1165-1168
|
@Anshgrover23 resolved relevant bot comments |
Signed-off-by: karan-palan <karanpalan007@gmail.com>
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Karan-Palan Quality Gate failing. Kindly fix it.
|
@Anshgrover23 The quality gate checks are nothing but test public IP address values, should not be an issue. Still pushing a commit trying to fix the CI |
Signed-off-by: karan-palan <karanpalan007@gmail.com>
|
The password and the ip addresses are standard RFC 1918 private/reserved IP ranges used exclusively in test fixtures. Sonar cloud suppression could be added in its settings |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/netplan_validator.py`:
- Around line 199-219: The validate_semantics method currently assumes config is
a mapping and will raise if the YAML root is a list/string; add an early type
guard at the top of validate_semantics to verify config is an instance of dict
(mapping) and if not return ValidationResult(False, ["Root configuration must be
a mapping/dictionary"], [], []); keep the rest of the function unchanged and
reference validate_semantics, the config variable, and the ValidationResult
constructor when adding this check to ensure non-mapping roots produce a clean
validation result instead of an exception.
- Around line 664-684: The current wait_for_input() catches
AttributeError/OSError but misses ImportError/ModuleNotFoundError from importing
termios/tty, preventing the fallback input() path on non-Unix systems; update
the function so the import of termios/tty is wrapped in a try/except that also
catches ImportError and ModuleNotFoundError (or catch Exception for the import
block) and, on import failure, runs the existing fallback that reads response =
input().strip().lower() and sets confirmed when "y"; keep the existing tty-based
logic intact for successful imports and preserve restoring terminal settings in
wait_for_input.
♻️ Duplicate comments (1)
tests/test_netplan_validator.py (1)
21-96: Add type hints to fixtures and helpers.The project requires type hints for Python code, including tests. Please annotate fixture parameters/returns.
♻️ Example pattern
-def temp_netplan_dir(tmp_path): +def temp_netplan_dir(tmp_path: Path) -> Path: @@ -def temp_backup_dir(tmp_path): +def temp_backup_dir(tmp_path: Path) -> Path: @@ -def valid_netplan_config(): +def valid_netplan_config() -> str: @@ -def invalid_yaml_config(): +def invalid_yaml_config() -> str: @@ -def invalid_ip_config(): +def invalid_ip_config() -> str: @@ -def netplan_validator(temp_netplan_dir, temp_backup_dir, valid_netplan_config, monkeypatch): +def netplan_validator( + temp_netplan_dir: Path, + temp_backup_dir: Path, + valid_netplan_config: str, + monkeypatch: pytest.MonkeyPatch, +) -> NetplanValidator:As per coding guidelines, type hints are required.
| def validate_semantics(self, config: dict[str, Any]) -> ValidationResult: | ||
| """ | ||
| Validate semantic correctness of network configuration. | ||
| Args: | ||
| config: Parsed YAML configuration | ||
| Returns: | ||
| ValidationResult with semantic validation results | ||
| """ | ||
| errors = [] | ||
| warnings = [] | ||
| info = [] | ||
|
|
||
| # Check for required 'network' key | ||
| if "network" not in config: | ||
| errors.append("Configuration must have a 'network' key") | ||
| return ValidationResult(False, errors, warnings, info) | ||
|
|
||
| network = config["network"] | ||
|
|
There was a problem hiding this comment.
Guard against non-mapping root configs.
If the YAML root is a list or string containing "network", the current logic can raise instead of returning a clean ValidationResult. Add a root type check up front.
🐛 Proposed fix
- # Check for required 'network' key
- if "network" not in config:
+ if not isinstance(config, dict):
+ errors.append("Configuration root must be a mapping/object")
+ return ValidationResult(False, errors, warnings, info)
+ # Check for required 'network' key
+ if "network" not in config:
errors.append("Configuration must have a 'network' key")
return ValidationResult(False, errors, warnings, info)🤖 Prompt for AI Agents
In `@cortex/netplan_validator.py` around lines 199 - 219, The validate_semantics
method currently assumes config is a mapping and will raise if the YAML root is
a list/string; add an early type guard at the top of validate_semantics to
verify config is an instance of dict (mapping) and if not return
ValidationResult(False, ["Root configuration must be a mapping/dictionary"], [],
[]); keep the rest of the function unchanged and reference validate_semantics,
the config variable, and the ValidationResult constructor when adding this check
to ensure non-mapping roots produce a clean validation result instead of an
exception.
| def wait_for_input(): | ||
| try: | ||
| import sys | ||
| import termios | ||
| import tty | ||
|
|
||
| fd = sys.stdin.fileno() | ||
| old_settings = termios.tcgetattr(fd) | ||
| try: | ||
| tty.setraw(sys.stdin.fileno()) | ||
| ch = sys.stdin.read(1) | ||
| if ch.lower() == "y": | ||
| confirmed.set() | ||
| finally: | ||
| termios.tcsetattr(fd, termios.TCSADRAIN, old_settings) | ||
| except (AttributeError, OSError) as e: | ||
| # Fallback for non-Unix systems or no TTY | ||
| # IOError/OSError: stdin issues, AttributeError: termios missing | ||
| response = input().strip().lower() | ||
| if response == "y": | ||
| confirmed.set() |
There was a problem hiding this comment.
Handle missing termios with ImportError fallback.
On platforms without termios, the import raises ImportError/ModuleNotFoundError, so the fallback input() path never runs and confirmation can’t be given.
🐛 Proposed fix
- except (AttributeError, OSError) as e:
+ except (AttributeError, OSError, ImportError, ModuleNotFoundError):
# Fallback for non-Unix systems or no TTY
# IOError/OSError: stdin issues, AttributeError: termios missing
response = input().strip().lower()🤖 Prompt for AI Agents
In `@cortex/netplan_validator.py` around lines 664 - 684, The current
wait_for_input() catches AttributeError/OSError but misses
ImportError/ModuleNotFoundError from importing termios/tty, preventing the
fallback input() path on non-Unix systems; update the function so the import of
termios/tty is wrapped in a try/except that also catches ImportError and
ModuleNotFoundError (or catch Exception for the import block) and, on import
failure, runs the existing fallback that reads response =
input().strip().lower() and sets confirmed when "y"; keep the existing tty-based
logic intact for successful imports and preserve restoring terminal settings in
wait_for_input.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Karan-Palan Address this Issues.
2 participants
Related Issue
fixes #445
Summary
Implements a comprehensive Netplan configuration validator to prevent network outages from simple YAML typos and configuration errors. Includes YAML/semantic validation, diff preview, dry-run with auto-revert, automatic backups, and plain English error messages.
Screencast.from.2026-01-23.18-03-06.mp4
Features Implemented
YAML syntax validation - Catches formatting errors with line/column numbers
Semantic validation - Validates IP addresses, routes, gateways, DNS servers
Configuration diff - Color-coded preview of changes
Dry-run mode - Apply with automatic revert timer (customizable timeout)
Plain English errors - User-friendly error messages
Bonus features:
Usage Examples
Validate configuration
All tests (newly added ones as well) pass using
make check:AI Disclosure
Claude code used to understand issue, generate tests and edge cases. All code has been personally audited and reviewed
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.