[dpd-api] migrate to RFD 619 pattern#259
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
| // | ||
| // TODO: `FreeChannels` is unchanged across versions, so this split may be | ||
| // unnecessary — the rationale for gating the endpoint at | ||
| // `VERSION_MCAST_STRICT_UNDERLAY` is not obvious from the code alone. | ||
| // Revisit in a follow-up to either document the reason or collapse back to | ||
| // a single `channels_list` endpoint. |
There was a problem hiding this comment.
Wanted to ask about this -- is this correct?
There was a problem hiding this comment.
We don't need the split. It was previously introduced because of a misspelling fix in the API doc comment, which shouldn't have warranted a version change (though it's official documentation). Collapsing it back to a single channels_list endpoint is totally the way to go.
| versions = VERSION_ATTACHED_SUBNETS.., | ||
| versions = VERSION_ATTACHED_SUBNETS.., |
There was a problem hiding this comment.
There were a few tabs which I switched over to spaces.
| // Required method: the latest version requires a `MulticastGroupTagQuery` | ||
| // query parameter that v1 does not have. When the tag is absent from the | ||
| // request body, the implementation must look up the existing group's tag |
There was a problem hiding this comment.
nit: v1's tag is always present in the body, but it's an Option vs "absent". What's missing is the concrete context type and its switch-specific ops. So maybe more like:
| // Required method: the latest version requires a `MulticastGroupTagQuery` | |
| // query parameter that v1 does not have. When the tag is absent from the | |
| // request body, the implementation must look up the existing group's tag | |
| // Required method: v1 carries `tag` as `Option<String>` in the body, while the | |
| // latest version moves it to a required `MulticastGroupTagQuery`. Resolving a | |
| // `None` body tag means reading the existing group's tag from switch state, | |
| // which is reachable only through the concrete context type in the implementation. |
| // Required method: the latest version requires a `MulticastGroupTagQuery` | ||
| // query parameter that v7 does not have. When the tag is absent from the | ||
| // request body, the implementation must look up the existing group's tag |
There was a problem hiding this comment.
nit: follow a similar suggestion to above?
| // TODO: `FreeChannels` is unchanged across versions, so this split may be | ||
| // unnecessary — the rationale for gating the endpoint at | ||
| // `VERSION_MCAST_STRICT_UNDERLAY` is not obvious from the code alone. | ||
| // Revisit in a follow-up to either document the reason or collapse back to |
There was a problem hiding this comment.
Adding this again in context of review:
We don't need the split. It was previously introduced because of a misspelling fix in the API doc comment, which shouldn't have warranted a version change (though it's official documentation). Collapsing it back to a single channels_list endpoint is totally the way to go (could do this quickly as a follow-up too).
zeeshanlakhani
left a comment
zeeshanlakhani
left a comment
There was a problem hiding this comment.
Just a couple nits.
2 participants
No description provided.