Skip to content

#569 - add check for minimum netbox/branching versions for branching support#570

Open
arthanson wants to merge 3 commits into
featurefrom
569-check
Open

#569 - add check for minimum netbox/branching versions for branching support#570
arthanson wants to merge 3 commits into
featurefrom
569-check

Conversation

@arthanson

@arthanson arthanson commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Fixes: #569

Adds a Django system check that enforces minimum NetBox and netbox-branching versions only when netbox-branching is installed.

@arthanson

Copy link
Copy Markdown
Contributor Author

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

Claude finished @arthanson's task in 1m 42s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze and post review

Concerns

1. No test coverage for checks.py

The new check has three meaningful branches (branching not installed → early return; NetBox version too low; branching version too low), plus the silent-pass paths. None are covered by tests. A failure in checks.py (e.g. a future refactor breaking the apps.is_installed guard) would go undetected until a user hits it at startup. Consider adding a test module — test_checks.py — that mocks apps.is_installed, settings.RELEASE, and _pkg_version to exercise each branch. Fix this →

2. PackageNotFoundError silently skips the branching version check for editable installs

checks.py:59 catches PackageNotFoundError and returns without an error. The comment acknowledges this ("editable install without dist-info — skip rather than warn"), but it creates a real gap: a developer running an incompatible editable netbox-branching checkout gets no warning at all. A Warning-level check (rather than silent pass) would at least surface the ambiguity:

except PackageNotFoundError:
    errors.append(Warning(
        'netbox-branching is installed but its version could not be determined.',
        hint='If using an editable install, ensure dist-info is present.',
        id='netbox_custom_objects.W001',
    ))

Fix this →


@arthanson

Copy link
Copy Markdown
Contributor Author

re claude review, fixed 1 and 2.

@arthanson arthanson marked this pull request as ready for review June 11, 2026 18:25
@arthanson arthanson requested a review from bctiemann June 11, 2026 18:26


# Version floors enforced only when netbox-branching is installed.
REQUIRED_NETBOX_VERSION = '4.6.2'

@bctiemann bctiemann Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this one pull from the min_version on the PluginConfig? Or else we'll have to keep this in sync too. Or if this is relevant only when branching is present, maybe REQUIRED_NETBOX_VERSION_FOR_BRANCHING or something? (If it's truly unrelated to the custom-objects min_version then I don't mind this.)

@bctiemann bctiemann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See inline

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