Skip to content

[management] no events for temporary peers#5719

Merged
pascal-fischer merged 2 commits intomainfrom
fix/no-event-for-proxy-peers
Mar 30, 2026
Merged

[management] no events for temporary peers#5719
pascal-fischer merged 2 commits intomainfrom
fix/no-event-for-proxy-peers

Conversation

@pascal-fischer
Copy link
Copy Markdown
Collaborator

@pascal-fischer pascal-fischer commented Mar 27, 2026

Describe your changes

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

  • Refactor
    • Adjusted activity event behavior so creating temporary peers no longer generates persistent creation events.
    • Suppressed user-deletion activity entries for embedded or Wasm-based peers while keeping deletions and policy-removal events intact.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

AddPeer now skips persisting activity events when temporary is true; peer deletion no longer enqueues PeerRemovedByUser events for peers with ProxyMeta.Embedded == true or Meta.KernelVersion == "wasm", while deletion and policy-removed events remain unchanged.

Changes

Cohort / File(s) Summary
AddPeer event persistence
management/server/peer.go
Skip calling StoreEvent(...) when temporary is true; event persistence still occurs for non-temporary peers.
Peer deletion event filtering
management/internals/modules/peers/manager.go
When deleting peers, do not enqueue activity.PeerRemovedByUser for peers where peer.ProxyMeta.Embedded is true or peer.Meta.KernelVersion == "wasm"; other deletion and policy event logic unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • bcmmbaga

Poem

🐰 I nudged a peer — it came and hopped away,
No heavy echoes left to clog the day.
Embedded friends and wasm leaves fall light,
Quiet deletions, soft as moonlit flight. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is largely incomplete: it contains only the repository template with empty sections for describing changes, issue links, and implementation details. Complete the description by filling in 'Describe your changes' with what is being changed and why, add the issue ticket number and link, specify the type of change (bug fix/feature/refactor), and explain why documentation is not needed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: preventing activity events for temporary peers across peer creation and deletion operations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/no-event-for-proxy-peers

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
management/server/peer.go (1)

1485-1485: Consider extracting the "wasm" magic string to a named constant.

The "wasm" kernel version check appears in both manager.go and here. Extracting it to a constant (e.g., peer.KernelVersionWASM) would improve maintainability and reduce the risk of typos.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@management/server/peer.go` at line 1485, Extract the magic string "wasm" into
a shared constant (e.g., KernelVersionWASM) so both manager.go and peer.go use
the same identifier; add the constant to the peer package (or appropriate
package where peer.Meta is defined), then replace occurrences of the literal
string checks (for example the condition using peer.ProxyMeta.Embedded and
peer.Meta.KernelVersion in peer.go and the corresponding check in manager.go)
with comparisons against peer.KernelVersionWASM to ensure consistency and avoid
typos.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@management/server/peer.go`:
- Line 1485: Extract the magic string "wasm" into a shared constant (e.g.,
KernelVersionWASM) so both manager.go and peer.go use the same identifier; add
the constant to the peer package (or appropriate package where peer.Meta is
defined), then replace occurrences of the literal string checks (for example the
condition using peer.ProxyMeta.Embedded and peer.Meta.KernelVersion in peer.go
and the corresponding check in manager.go) with comparisons against
peer.KernelVersionWASM to ensure consistency and avoid typos.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3e6dbb8a-238c-44fe-b86f-ff4f27e24de7

📥 Commits

Reviewing files that changed from the base of the PR and between 6049262 and 4e84f1b.

📒 Files selected for processing (2)
  • management/internals/modules/peers/manager.go
  • management/server/peer.go

@pascal-fischer pascal-fischer merged commit be6fd11 into main Mar 30, 2026
63 of 64 checks passed
@pascal-fischer pascal-fischer deleted the fix/no-event-for-proxy-peers branch March 30, 2026 08:08
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