Skip to content

Introduced an option to disable physical nic updates in Netbox - #442

Merged
semx merged 2 commits into
bb-Ricardo:developmentfrom
pertoft:main
Sep 9, 2026
Merged

semx merged 2 commits into
bb-Ricardo:developmentfrom
pertoft:main

Conversation

@pertoft

@pertoft pertoft commented Feb 26, 2025

Copy link
Copy Markdown

We have issues today with the netbox sync, as it will mess with host physical networks.
Therefore I have introduced an config option to disable host network updates.

@bb-Ricardo
bb-Ricardo changed the base branch from main to development February 26, 2025 10:32
@bb-Ricardo
bb-Ricardo force-pushed the development branch 2 times, most recently from 8adc4cd to 4c5fbc8 Compare March 9, 2026 09:09
@bb-Ricardo
bb-Ricardo self-requested a review as a code owner May 13, 2026 22:13
@semx
semx merged commit 0ed4680 into bb-Ricardo:development Sep 9, 2026
@bb-Ricardo

Copy link
Copy Markdown
Owner

@semx: are you sure this works as intended? As far as I can see:

  • the update example config is missing
  • and my comment regarding skipping the whole host sync if this options is set to true has not been answered by the author.

@semx

semx commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

You are right on both points, and I should have exercised the option before merging rather than only the default path. Verified now with the vcsim suite: with skip_host_nics = True the early return in add_host() dropped the whole host, since it sat before the host is added to the inventory.

#555 fixes that and adds the missing settings-example.ini entry: the option now skips only the physical interfaces; the host, its VMkernel interfaces and the VMs are synced as before (the vswitch/port-group data is still collected because VM interface parsing depends on it). A test that fails on development and passes with the fix is included.

One semantic point for you to decide: physical interfaces that already exist in NetBox are simply no longer updated by the source once the option is on, so they are treated as unreported by the prune logic. If you would rather have the option leave existing pNICs untouched entirely, or skip the VMkernel interfaces too, I'll adjust #555 accordingly. Happy to revert #442 instead if you prefer the author to finish it.

@bb-Ricardo

Copy link
Copy Markdown
Owner

For PRs which are not mergeable but still considered useful, I would suggest following:
• creating a new branch from development with a fitting name
• change the PR taget to the new branch
• creat a new PR from the new branch against development and add necessary fixes

@semx

semx commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Understood, I'll use that flow from now on: a feature/<name> branch from development, the contributor's PR retargeted and merged there, fixes added on the branch, and one PR from the branch to development. For this one the fixes are in #555 (the option now skips only the physical interfaces; example config added; test included). I'm applying the same flow to the other contributor PRs that are currently waiting on rebases (#552 first, then #518, #514, #535) instead of asking the authors to rebase.

@bb-Ricardo

bb-Ricardo commented Sep 9, 2026

Copy link
Copy Markdown
Owner

It is quite some work but in some cases it is useful to keep the changes already implemented and fix the missing parts rather then waiting for weeks/months for feedbacks or upfates.

Thank you again for all this work.

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.

4 participants