fix(drift): run one reversed octodns-sync per live provider - #5
Conversation
generate_drift_config() reversed each zone's full targets list into a single sources list, so a multi-target zone (e.g. hcloud + desec) populated one octoDNS zone object from every live provider at once. octoDNS populates with lenient=False, so any record present in more than one provider raises DuplicateRecordException — and with a shadow-provider setup every shared record collides, making drift-check structurally unusable for such repos. Now: - live_providers() collects all live providers across zones (first-appearance-ordered, deduplicated) - generate_drift_config() takes a required provider argument, scopes the reversed config to zones targeting that provider, and returns the reversed zones mapping - main() runs octodns-sync once per provider with its own temp config, fail-fast rc 2 on error, aggregates drift per provider, and reports each drifting provider by name; --zone skips providers that do not serve that zone Fixes #4
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ea2985094
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- A bare `targets:` key loads as None; iterate `or []` like octoDNS's own manager instead of crashing with TypeError (P2). - The --zone provider skip is only decidable when every zone key in the provider's reversed config is concrete: a '*'-prefixed key is a dynamic zone entry only octoDNS can expand, so with one present the run proceeds and octodns-sync applies the zone filter itself (P1 -- previously --zone on a dynamic-zone config skipped every provider and falsely reported "No drift detected").
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe8a3bac89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…check Second review round on PR #5, all three findings in the per-provider config generation: - Zones not targeting the provider are now kept as inert blockers (original sources, targets: []) instead of being dropped: octoDNS skips them without populating ("no eligible targets"), but the key still subtracts from a dynamic ('*') entry's expansion candidates, so an explicit zone that deliberately omits a provider no longer gets re-included by that provider's wildcard. - With every zone key present, the --zone provider precheck is unnecessary and is removed: the filter is passed straight to octodns-sync, whose IdnaDict lookup normalizes case/IDNA (the raw dict membership test here did not) and whose manager expands dynamic zones before filtering. A provider not serving the zone now resolves to its blocker entry and reports no changes. - manager.plan_outputs is stripped from the generated configs: it writes to a fixed filename, so one run per provider would leave only the last provider's plan. Drift reads the sync stderr; file plan outputs serve no purpose here.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- A bare `targets:` key loads as None; iterate `or []` like octoDNS's own manager instead of crashing with TypeError (P2). - The --zone provider skip is only decidable when every zone key in the provider's reversed config is concrete: a '*'-prefixed key is a dynamic zone entry only octoDNS can expand, so with one present the run proceeds and octodns-sync applies the zone filter itself (P1 -- previously --zone on a dynamic-zone config skipped every provider and falsely reported "No drift detected").
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06288a0e7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reversed_zones[zone_name] = { | ||
| "sources": zone_cfg.get("sources") or [], | ||
| "targets": [], | ||
| } |
There was a problem hiding this comment.
When a dynamic entry not targeting this provider uses glob or regex, rebuilding it with only sources and targets discards that filter. Because octoDNS expands dynamic entries in order and subtracts prior matches, this blocker can consequently expand to every zone and leave a later dynamic entry for the current provider with no candidates—for example, a filtered p1 pattern followed by a filtered p2 pattern—so p2 is never checked and the command can incorrectly report no drift. Copy the dynamic selector fields into the blocker so it excludes exactly the zones selected by the original entry.
Useful? React with 👍 / 👎.
1 participant
Fixes #4.
Problem
generate_drift_config()reversed each zone's fulltargetslist into a singlesourceslist. For a multi-target zone (e.g.targets: [hcloud, desec]), octoDNS then populated one zone object from every live provider withlenient=False— any record present in more than one provider raisesDuplicateRecordException. With a shadow-provider setup (Hetzner mirroring deSEC) every shared record collides, so drift-check could never run for such a repo. The record named in the exception is just whichever shared record loads first.Fix
One reversed plan per live provider:
live_providers(zones)— all live providers across zones, deduplicated, first-appearance order.generate_drift_config(config_path, output_path, provider)—provideris now required; the reversed config includes only zones targeting that provider; returns the reversed zones mapping.main()loops octodns-sync once per provider, each with its own temp config. Errors fail fast (rc 2). Drift is aggregated per provider and reported with the provider named.--zoneskips providers whose reversed config lacks that zone. rc 0 + "No drift detected" only when every provider is clean.Single-target repos see identical behavior (one provider, one run).
Tests
providerargument.live_providersordered dedup; per-provider config scoping (zone exclusion +sourcescontent);main()running sync once per provider with distinct config files; drift in one provider → rc 1 naming it; error in first provider → rc 2, no second run.Note: this PR was pushed without a pre-commit human review pause; review on the draft.