oxidecomputer / oxidecomputer/omicron

`BgpPeer::communities` - use a set instead of a vec?

Open Beginner friendly
#10,138 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

BgpPeer::communities is currently a Vec<u32>: https://github.com/oxidecomputer/omicron/blob/6a3bdd6a28b4d831bc12337d3c4bdafc730ab385/nexus/types/versions/src/bgp_unnumbered_peers/networking.rs#L163-L164

However, I think it would be more accurate to make this a BTreeSet<u32>:

  • Inserting and then reading back from the db does not preserve order
  • We don't expect duplicate values - if the Vec has duplicates in it, insertion will fail with an ObjectAlreadyExists error

(Like #10121, this came up while extending tests in support of the #9832 work.)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in nexus/types/versions/src/bgp_unnumbered_peers/networking.rs at the BgpPeer::communities definition. Review the tests added for the #9832 work and the database insertion behavior described in this issue. Done means the communities representation reflects unordered, duplicate-free values and the relevant tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
databases
Issue type
Refactor
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.