Monorepo for nightops#19
Conversation
There was a problem hiding this comment.
Pull request overview
Adds monorepo workspace scaffolding and improves “out-of-the-box” behavior by loading default config/policy YAML from packaged resources when no filesystem config exists.
Changes:
- Load packaged default remediation policies and default
nightops.yamlviaimportlib.resources. - Include the
config/directory in the built wheel and adjustpyproject.tomllicense metadata. - Add a
monorepo/workspace layout with docs and apackages/nightopspointer.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/remediation/policy_engine.py | Fallback to packaged remediation policy YAML when no local policy file exists. |
| src/core/config.py | Support loading packaged default config YAML and add YAML-text loader with env substitution. |
| pyproject.toml | Include config/ in wheel build and update license metadata format. |
| monorepo/packages/nightops | Adds a “pointer” to repo root for monorepo workspace usage. |
| monorepo/README.md | Documents monorepo setup, CLI usage, and publishing steps. |
Comments suppressed due to low confidence (1)
monorepo/packages/nightops:1
monorepo/README.mddescribesmonorepo/packages/nightopsas a symlink, but this appears to be a regular file containing../../. That won’t behave like an installable project directory forpip install -e "packages/nightops[dev]". Either commit an actual symlink (Git supports this) or switch to a documented approach that doesn’t require symlinks (e.g., a path dependency / editable install pointing at../..) and update the README accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def from_yaml_text(cls, raw: str) -> NightOpsConfig: | ||
| """Load configuration from YAML text with environment variable substitution. | ||
|
|
||
| This is used for packaged-in defaults where there is no filesystem path | ||
| alongside the YAML file (so we can't load an adjacent `config/.env`). | ||
| """ | ||
| # Substitute environment variables (${VAR_NAME} syntax) | ||
| for key, value in os.environ.items(): | ||
| raw = raw.replace(f"${{{key}}}", value) | ||
|
|
||
| # Replace any remaining unresolved ${VAR} references with empty string | ||
| raw = re.sub(r"\$\{[A-Za-z_][A-Za-z0-9_]*\}", "", raw) | ||
|
|
||
| data = yaml.safe_load(raw) | ||
| data = yaml.safe_load(raw) or {} |
There was a problem hiding this comment.
Environment substitution + YAML parsing + mcp_servers normalization now exists in both from_yaml and from_yaml_text, which increases the chance they drift (the or {} fix already differs between the two). Consider extracting a shared internal helper (e.g., _parse_yaml_config(raw: str) -> dict) that both entry points call, keeping file-specific behavior (like reading .env relative to a path) separate.
| [tool.hatch.build.targets.wheel.force-include] | ||
| "src" = "nightops" | ||
| "config" = "nightops/config" | ||
|
|
There was a problem hiding this comment.
The runtime code now depends on packaged resources (nightops/config/nightops.yaml). This change force-includes config/ for wheels, but it may still be missing from sdists depending on hatch configuration, which can break installs that build from sdist. Consider also including config/ in the sdist target (or otherwise ensuring all package builds include these YAML resources).
| [tool.hatch.build.targets.sdist] | |
| include = [ | |
| "src", | |
| "config", | |
| ] |
| @@ -0,0 +1,137 @@ | |||
| # Monorepo Layout (Local Workspace) | |||
|
|
|||
| This repo is already packaged as a Python distribution named `nightops` (see `/Users/suchitraswain/Documents/google/TheNightOps/pyproject.toml`). | |||
There was a problem hiding this comment.
The README hard-codes an absolute local filesystem path, which won’t apply to other developers/CI and makes the instructions non-portable. Replace with repo-relative paths (e.g., “from the repository root”) and, if needed, show examples using $REPO_ROOT or pwd-based paths.
|
@SuchitraSwain review this |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
monorepo/packages/nightops:1
- This appears to be a Git symlink (stored as a file containing the link target). Symlinks can be problematic on Windows and in some packaging/checkout workflows (zip downloads, restricted filesystem permissions), which may break the documented
pip install -e \"packages/nightops[dev]\"flow. If cross-platform support is needed, consider an alternative (e.g., a smallpackages/nightops/pyproject.tomlthat depends on the root package, or workspace tooling that doesn’t require symlinks), or explicitly document the symlink requirement/limitations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for key, value in os.environ.items(): | ||
| raw = raw.replace(f"${{{key}}}", value) | ||
| raw = re.sub(r"\$\{[A-Za-z_][A-Za-z0-9_]*\}", "", raw) |
There was a problem hiding this comment.
The two-pass env substitution can corrupt legitimate values that contain ${...} after replacement (e.g., if an env var value itself contains ${HOME}, the subsequent regex will delete it). A safer approach is a single re.sub pass with a callback that replaces only ${NAME} tokens by looking up NAME in os.environ (defaulting to empty string when missing), and avoids scanning/replacing for every env var.
| for key, value in os.environ.items(): | |
| raw = raw.replace(f"${{{key}}}", value) | |
| raw = re.sub(r"\$\{[A-Za-z_][A-Za-z0-9_]*\}", "", raw) | |
| pattern = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}") | |
| raw = pattern.sub(lambda m: os.environ.get(m.group(1), ""), raw) |
| @@ -140,6 +142,47 @@ def _load_policies(self, path: str) -> None: | |||
| """Load policies from a YAML file, merging with defaults.""" | |||
| policy_file = Path(path) | |||
| if not policy_file.exists(): | |||
There was a problem hiding this comment.
The packaged-default lookup reuses the user-supplied path string verbatim. If callers pass an absolute filesystem path (or a path with drive letters / path separators), the join against package resources becomes ambiguous and can mask a simple 'file not found' misconfiguration. Consider only attempting packaged lookup for expected relative default locations (e.g., when not policy_file.is_absolute()), or normalize to a known resource-relative path before joining.
| if not policy_file.exists(): | |
| if not policy_file.exists(): | |
| # Only attempt packaged lookup for non-absolute paths to avoid | |
| # ambiguities when users pass absolute filesystem locations. | |
| if policy_file.is_absolute(): | |
| logger.info("No policy file at %s, using defaults", path) | |
| return |
No description provided.