feat(triggers): do nothing while netbox-branching merges, reverts or syncs - #161
Merged
Merged
Conversation
…syncs netbox-branching merges, reverts and syncs a branch: it replays the changes that NetBox logged. A replayed update is a model save (netbox_branching/utilities.py:513), and a replayed create is a raw save (models/changes.py:131). So the plugin's receivers ran for each replayed module and device. After a merge, the reapply ran at the commit with the rules of main and outside event tracking: it renamed on main with no change record. During a sync, it renamed in the branch with no change record, so a merge never carried the rename. At startup, after the version check, the plugin now wraps Branch.merge, Branch.revert and Branch.sync once. Each wrapper marks its context with a ContextVar and resets the token in a finally, so the mark ends on a return, an early return and an error. While the mark is set, before_save and after_save return at once: no previous-state read, no trigger and no plan. The netbox-branching jobs call these methods (jobs.py:132, 217, 243), and the merge strategies run inside them (models/branches.py:1135-1139, 1211-1214). Rejected alternatives: - The pre_ and post_ signals. netbox-branching sends no post_ signal on the "No changes found" return (models/branches.py:923-924, 1120-1121, 1196-1197) or on an error (988-993, 1144-1150, 1219-1225). - A reset from the branch status. It reads a status that other workers share, and it adds one query for each trigger. - Skipping raw saves. It changes a fixture load on main. A merge or revert started in a shell with the branch active no longer raises at the alias check of the trigger. netbox-branching itself still refuses some replays there: its full_clean validates a replayed create against the active branch (models/changes.py:110), and the revert of a module move fails at COMMIT on the AppliedChange foreign key (utilities.py:525). That revert fails also with the plugin's receivers disconnected. The tests start each operation on netbox-branching's page and run its job from the queue, as a worker does: - A merge of an install and a move gives main the names of the branch, and main logs only replayed changes. A revert gives back the names from before. The rule exists on main only, so a trigger would rename. Both with the iterative and the squash strategy. - A sync writes no rename into the branch. The rule exists in the branch only. - After a merge without changes, a dry run, a failed merge, and a dry run followed by another worker's merge of the branch, an install on main still gets the names of the rule. - NetBox's channel cascade (dcim/models/mixins.py:302-337) renames a kept channel again at the commit of a merge and of a sync. A revert gives back the names from before. This pins the accepted limit. - A contract test checks that each method is wrapped once and keeps its signature, name and alters_data. The branch write tests now share one reader of the interfaces of a bay, on the active branch or on main. Each mutation turns tests red: no mark (the merges, the reverts, the sync and the active-branch test), a reset without a finally (the dry run and the failed merge), the signals with the status reset (the other-worker test), the signals alone (the three exit tests), skipped raw saves (the fixture load on main), and a second wrap (the contract test). Slice 6 of #143 (ADR 0016).
The configuration guide now has a netbox-branching section. It states the supported release, 1.2.x on NetBox 4.7, and that NetBox does not start with another release. It describes what the plugin does in a branch: the rename triggers, Apply Rules, the conversion and the jobs run in the branch. A script installs a module in a transaction on the interface write connection, as the Apply Rules section already says. In a branch, each plugin operation sets a lock_timeout of 10 seconds on both connections. An engine function that a script calls inside its own transaction leaves the lock waits of the commit callbacks to the script. The section describes merge, revert and sync. The rename triggers do nothing while netbox-branching replays changes. A merge gives main the names of the branch, a revert gives back the names from before, and a sync writes no rename without a change record. It lists the accepted limits of the design record: - NetBox's channel cascade runs again at the commit of a replay (dcim/models/mixins.py:302-337). - A bulk REST request with background=true runs on main, because the background job does not keep the cookies of the request (netbox/api/viewsets/mixins.py:307, 345-347). The rule endpoints refuse it in a branch; the other endpoints keep the gap. - NetBox ignores a failed branch activation (utilities/request.py:132-141). - There is no atomicity across the two connections. - PgBouncer in transaction mode is not supported. The installation guide lists netbox-branching as an optional requirement. Its upgrade note says to let the queued plugin jobs finish before the upgrade, because a job that an earlier release queued fails: the jobs now store their branch. The engine example states the lock timeout of a call inside a transaction of the caller, and tells the script to set lock_timeout for the commit callbacks. The README and the index list the feature. They and the merge and sync statements point at the limit of a kept channel. Documentation tests pin the supported series in the configuration guide, the installation guide, the README and the index to the constant of the version gate. They also pin the replay statement with the cascade limit, and the upgrade note. A helper reads a whole page with its whitespace collapsed, at each site in the documentation tests. Slice 6 of #143 (ADR 0016).
…tions The architecture list named no owner of the database connections and no module for netbox-branching. It now names transactions.py, the one owner of connections and transaction state, and branching.py, the one module that imports netbox-branching: the version check, the branch of a job, and the mark that stops the rename triggers during a merge, revert or sync. Part of #143 (ADR 0016).
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (32)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe plugin now wraps netbox-branching merge, revert, and sync operations, and skips rename triggers during replay. Rule saves check database routing outside replay. Shared test helpers replace duplicated fixtures, and documentation describes compatibility, branch operations, and limitations. ChangesBranch replay and write routing
Shared test support
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant BranchMethods as Branch.merge/revert/sync
participant ReplayWrapper as branching wrapper
participant ReplayFlag as replay context flag
participant SaveHandlers as rename_triggers
BranchMethods->>ReplayWrapper: call replay method
ReplayWrapper->>ReplayFlag: set replaying
ReplayWrapper->>BranchMethods: run original replay
BranchMethods->>SaveHandlers: invoke save handlers
SaveHandlers->>ReplayFlag: check replay_in_progress
SaveHandlers-->>BranchMethods: return without rename-trigger work
ReplayWrapper->>ReplayFlag: restore prior flag
Merge Risk: ⚪ Minimal · up to The replay handling preserves branch operations while ordinary saves retain routing checks. No merge-blocking defect is established; merge after normal checks, observing the documented kept-channel and cross-connection limitations. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The replay exception is deliberately scoped to branch operations and is cleared when they return or fail. Ordinary rule saves gain stronger routing checks. No exploitable security regression was established, but authorization and failure behavior in the branching dependency were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 519 functions across 34 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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. A rabbit checks the branch at dawn Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The plugin marks a replay only inside Branch.merge, Branch.revert and Branch.sync. The version gate accepts any 1.2.x release, so a later release could add a replay path outside those methods, and the rename triggers would then act during that replay. The test parses the source of the installed netbox-branching with ast. It finds each call of a replay primitive and the function that holds the call, and compares the set with an allow-list. Each entry names the wrapped method that reaches it. - The calls that save a replayed object: ObjectChange.apply and .undo (merge_strategies/iterative.py:34, 53; merge_strategies/squash.py:223, 271; models/branches.py:743-759, 877), and update_object and deserialize_object inside them (models/changes.py:107-140, 182-186). - The calls that start a replay: the strategy in Branch.merge and Branch.revert (models/branches.py:1139, 1214), the two sync helpers in Branch.sync (966, 975), and the jobs that call the wrapped methods (jobs.py:217, 243). The scan does not pin save() and delete(): netbox-branching calls them for its own Branch and ChangeDiff rows, and each save of a replayed object runs inside ObjectChange.apply, ObjectChange.undo or update_object. A copy of the installed package with one added function that calls change.apply() turns the first test red and names the new call site. A copy with an added caller of strategy.merge() turns the second test red. The installed 1.2.1 tree passes. A third test checks the scan on a constructed package on every leg. Follow-up of #143 (ADR 0016).
…st modules Test modules imported constants, builders and test case bases from other test modules, and several modules kept their own copy of a shared constant: PLAIN_TYPE had six definitions. Each name that a test module imported from another test module now lives in one shared module, under a public name: - helpers.py: the interface types, the breakout modes, TEST_PASSWORD, PLUGIN_LOGGER, the skip reasons, the builders of devices and module types, install_form, names_of, request_context, WriteInterfacesTo, ChannelizationTestCase and VcDriftTestCase. - trigger_cases.py: the move fixture, ModuleMoveTestCase, BayEditTestCase, flat_rule, the journal reasons, and the probes that count reapplies, read the journal and inject database failures. - branch_cases.py: BranchTestCase, the branch write cases, the branch values of lock_timeout and FLAT_NAMES. Like helpers.py, it imports netbox-branching only inside functions. The copies go: PLAIN_TYPE, CHANNEL_TYPE, PARENT_TYPE, PLUGIN_LOGGER, TEST_PASSWORD, FLAT, CHANNELIZED, REQUIRES_CHANNELIZATION, REQUIRES_NO_CHANNELIZATION, _journal, _module_reapplies and _reject_interface_updates. The breakout modes come from BreakoutModeChoices. Two skip reasons that lacked "(4.7+)" now have it. A local variable that would hide module_reapplies is renamed, and a moved two-line comment is one line. The shared modules do not match test_*.py, so pytest collects nothing from them. A base that a test module imports defines no test, so it adds no item. On the branch stack, --co collects the same 1695 test IDs before and after. No guard test or allow-list needed a change. Follow-up of #143.
…ackage Test modules imported constants, builders and test case bases from one another, and copies of shared constants grew next to them. The previous commit moved every shared name into helpers.py, trigger_cases.py and branch_cases.py. This guard keeps it that way. It parses each module of the test package and reports each import statement that names a test_* module of the package: import a.tests.test_x, from a.tests.test_x import y, from a.tests import test_x, and the relative from .test_x import y and from . import test_x, also inside a function. It reads import statements only, so a dotted-path string such as a MIDDLEWARE setting is not an import. A shared module, a test_* name imported from a shared module, and a package with a similar name are not reported. With "from .test_channelization import CHANNEL_TYPE" added to test_bulk_families.py, the guard fails and names that file and statement. The tree passes. Follow-up of #143.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…cope The replay call-site scan recorded only direct calls, as a set of (name, module, function). A later 1.2.x release could add an unwrapped replay path and both allow-list tests would stay green: - A call through an indirect reference: replay = change.apply, then replay(branch); getattr(change, "apply")(branch); or an import alias such as from .utilities import update_object as u. - A second receiver in an allowed scope: strategy.merge(...) next to the allowed branch.merge(...) in MergeBranchJob.run gave the same tuple. - A deferred call: transaction.on_commit(lambda: strategy.merge(...)) in Branch.merge ran after the wrapper reset its mark and gave the same tuple. The scan now records each reference to a tracked name: an attribute, a name, an imported name, and the string of a getattr, setattr or hasattr call. Each site is (name, receiver, module, scope). The receiver is the object of the attribute or the call, the source of the import, or empty. A lambda opens a scope of its own, as a function does. The tests compare a Counter, so a second occurrence in an allowed scope changes the result. A failure lists each added or missing site. The allow-lists now hold a count and a note for each site. The real 1.2.1 tree adds these references, each read in the source: - apply.alters_data and undo.alters_data (models/changes.py:156, 228), merge.alters_data and revert.alters_data (models/branches.py:1171, 1247): attribute flags, no call. - The imports of deserialize_object (models/changes.py:15) and update_object (models/changes.py:17-22). - hasattr(model, 'deserialize_object') in ObjectChange.apply (models/changes.py:106). - The revert flag of ObjectChange.migrate (models/changes.py:67-74), a parameter, not the method. Each finding has a regression case in the scan self-test. The three cases failed against the earlier scan and pass now. On copies of the installed package, the earlier scan stays green and the new one fails naming the added site for each of: an indirect reference, a getattr string, an import alias, a second receiver in MergeBranchJob.run, and a lambda in Branch.merge. Both scans fail on the two earlier mutations, and both pass on the installed 1.2.1 tree. Follow-up of #143 (ADR 0016).
The reference scan cannot see every way to defer a replay past the wrapper. A stored reference called from a lambda (replay = change.apply, then on_commit(lambda: replay(branch))) and a generator ((change.apply(branch) for change in changes)) inside an allowed function give the same references as the direct call. Each new syntax form needs its own rule, and a missed form stays green. So the test now fails closed. It records a fingerprint of each scope that the allow-lists name: a hash of the source of the function, or of the statements of the class or the module outside its nested scopes, from the AST, without positions or comments. A change to the body of any of those 17 scopes on a netbox-branching update fails the test, and the message tells the reader to read the scope again and to update its fingerprint and the allow-lists together. A reference outside the allowed scopes still fails as a new site, and the reference multiset still checks the receiver and the count. The scan and the fingerprint name scopes through one walk. The scan self-test has a case for each deferral: the references stay equal and the fingerprint changes. With the scan of the previous commit, the same two programs give equal Counters, so that check stays green. On copies of the installed package, the deferral by a stored reference in IterativeMergeStrategy.merge and the generator in Branch._handle_sync_delete leave the references unchanged: the earlier checks pass, and the new test fails naming the changed scope. The installed 1.2.1 tree passes. Follow-up of #143 (ADR 0016).
…__.py included The guard against imports of test modules read only the top-level files of the test package and skipped __init__.py. It resolved each relative import against the test package, and it checked only the first component under it. So these stayed green: - from .test_views import ViewTest in tests/__init__.py; - an import in a module of a test subpackage; - import netbox_interface_name_rules.tests.sub.test_views as views; - from netbox_interface_name_rules.tests.sub import test_views; - from ..test_views import ViewTest in tests/sub/shared.py, which resolved against the wrong package. The guard now reads each .py file under the test package, __init__.py included. It derives the package of each module from its path and resolves a relative import against that package. It reports an import whose target has a test_* component at any depth under the test package, and the name of a from-import counts as part of the target. So a test_* name imported from a shared module is refused too: pytest collects a test function where it is imported. With temporary imports in tests/__init__.py, in a new tests/sub subpackage and in helpers.py, the earlier guard passed and the new one fails naming the four statements. The new detector subtests for the two absolute subpackage spellings and the test_* name failed with the earlier detector. New tests cover a relative import from a subpackage, and __init__.py and subpackage modules in a temporary test package. The tree passes. Follow-up of #143.
A new upstream function that dispatches with getattr(change, action) (branch), where action is a variable, adds a replay path outside every reviewed scope. Neither the reference counts nor the fingerprints change. This is the third finding of one family: each scan rule misses the next form of dispatch. So the contract now answers the question at its source: is the installed netbox-branching the release that was reviewed? A test asserts that the installed version equals REVIEWED_NETBOX_BRANCHING, 1.2.1. When it differs, the message says what to re-review: run the scan, read every new reference, every changed fingerprint and any dynamic dispatch such as getattr with a variable name, then update the release, the fingerprints and the allow-lists together. The scan and the fingerprints stay, as the aid for that review. branching.installed_version() now reads the version, for the version gate and for this test. The gate still accepts any 1.2.x release. With the constant set to 1.2.2, the test fails with that message. With 1.2.1, it passes on the branch leg. Follow-up of #143 (ADR 0016).
With __all__ = ["test_views"] in tests/__init__.py, from . import * in another module of the test package loads tests.test_views, and the guard reported nothing: the import names no test module. The guard now refuses each wildcard import whose source resolves inside the test package. It does not read __all__. No module of the test package has such an import; the one wildcard import under tests, in isolated_settings.py, imports netbox.settings. The detector reports from . import *, from .helpers import * and from netbox_interface_name_rules.tests import *. A wildcard import from outside the test package is not reported. The earlier detector missed the three spellings, and the __all__ case in a temporary test package. Follow-up of #143.
InterfaceNameRule.save() compared the save alias with the alias that the router gives for a rule only when update_fields named a validated field. A full save and a targeted save of other fields (for example description) skipped the check. In a netbox-branching branch, rule.save(using="default") on a fully loaded rule therefore wrote the main row, while the same save of name_template raised. The comparison now runs before the update_fields branching, so every save refuses an alias that is not the routed write alias, before any query. The write scope membership check stays inside the locked block. Tests cover a full save and a description save through default in a branch, and through an unknown alias on main.
A merge or revert started from a shell with the branch active replays the logged changes on default, while the router gives the branch alias. netbox-branching replays a rule update through ObjectChange.apply(), update_object() and instance.save(using="default"). The rule save now checks the write alias on every save, so this replay raised RuntimeError, the same failure that the rename triggers had before they returned early during a replay. InterfaceNameRule.save() now skips its write-alias comparison while replay_in_progress() is true. The write scope check inside the locked block stays: default is always in the scope. The new test changes a rule through the REST API in the branch, then merges and reverts it with the branch active.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #143, slice 6, the last slice (design:
docs/design/netbox-branching.mdr6, ADR 0016).Why
netbox-branching merges, reverts and syncs a branch by replaying NetBox's change records. A replayed update is a model save (
netbox_branching/utilities.py:513), and a replayed create is a raw save (models/changes.py:131). So the plugin's rename triggers ran for each replayed module and device:Changes
branching.py:ready()runs the version check, then wrapsBranch.merge,Branch.revertandBranch.synconce (functools.wraps, marked withREPLAY_MARK).finally, so the mark ends on a return, an early return and an error.replay_in_progress()reads the mark.jobs.py:132, 217, 243), and the merge strategies run inside them.rename_triggers.py:before_saveandafter_savereturn at once while a replay runs: no previous-state read, no alias check, no plan.docs/configuration.md: a new netbox-branching section covers:lock_timeout;docs/installation.md: netbox-branching as an optional requirement, and the upgrade note: let queued plugin jobs finish first, because slice 5 changed what a job stores.transactions.pyandbranching.py.Tests
The tests start each operation on netbox-branching's page and run its job from the queue, as a worker does.
Merge (iterative and squash): an install and a move in the branch give main the branch's names. Main logs only replayed changes. The rule exists on main only, so a trigger would rename.
Revert (both strategies): main gets the names from before.
Sync: no rename lands in the branch. The rule exists in the branch only.
Early exits: after a merge with no changes, a dry run, a failed merge, and a dry run followed by another worker's merge, an install on main still gets the rule's names.
Active branch: a merge and a revert started from a shell with the branch active no longer raise.
Accepted limit: NetBox's channel cascade (
dcim/models/mixins.py:302-337) renames a kept channel again at the commit of a merge or a sync. A revert gives back the names from before.Contract: each method is wrapped once and keeps its signature
(self, user, commit=True), its name andalters_data.Docs: the supported series in the configuration guide, the installation guide, the README and the index is pinned to the version gate's constant. The replay statement, the cascade limit and the upgrade note are pinned too.
Rejected alternatives from the design, each caught by a test when mutated in:
finally;pre_/post_signals with a status-based reset (netbox-branching sends nopost_signal on the "No changes found" return or on an error);Results:
The combined coverage of the 4.5.3 leg and the branch leg, as CI computes it, is 98.93%.
query_counts.jsonis unchanged.Review
ObjectChange.apply/undo,update_object, the merge, revert and sync jobs,recover) runs inside the three wrapped methods.finally), and a patch-level pin (the design pins 1.2.x).netbox-branching behaviour (not the plugin's)
With its own branch active, netbox-branching still refuses some replays:
full_cleanvalidates a replayed create against the active branch (models/changes.py:110).utilities.py:525). It fails with the plugin's receivers disconnected too.The active-branch test therefore uses a virtual-chassis position change.
Follow-ups done in this PR
tests/test_branching.py):REVIEWED_NETBOX_BRANCHING = "1.2.1"). Any other release fails until its replay paths are re-reviewed. The production version gate still accepts 1.2.x;branching.installed_version()is the one reader of the version.getattr, a second call next to an allowed one, lambdas, generators, and dispatch through a variable. The version pin closes that family; the scan stays as the checklist for the next release.tests/helpers.pyand two new modules,tests/trigger_cases.pyandtests/branch_cases.py. This removed the sixPLAIN_TYPEcopies and the other duplicate constants.test_module_boundaries.pyrefuses an import of atest_*module anywhere in the test package, in any spelling, and refuses a wildcard import from the package, so this debt cannot come back.Summary by CodeRabbit
New Features
Bug Fixes
Documentation