Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect interface changes and start/stop new/removed lldp discovery #73

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

majst01
Copy link
Contributor

@majst01 majst01 commented Jul 15, 2022

Fix for #55 and #3

TODO:

  • check if switchport modifications trigger required actions

@majst01 majst01 marked this pull request as ready for review July 18, 2022 07:00
@majst01 majst01 requested a review from a team as a code owner July 18, 2022 07:00

func (c *Core) startLLDPDiscovery(ctx context.Context, discoveryResultChan chan lldp.DiscoveryResult, iface net.Interface) {
// consider only switch port interfaces
if !strings.HasPrefix(iface.Name, "swp") {
Copy link
Contributor

Choose a reason for hiding this comment

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

In cmd/internal/core/detect-changes.go line 32 the interface is checked for "swp" prefix.
If the same check gets kept in cmd/internal/core/phone-home.go then behavious is consistent across callers (both only call this function for "swp" interfaces) and the duplicate check here could be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is a bit too spread, will think of a solution, maybe factoring interface listing to a separate func (which also detects switch os in the future, SONiC hast "Ethernet.." )

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check the PR #68 for a possible solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

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.

3 participants