Refactor FullyQualifiedAuthority::normalize to always return authority #476
Conversation
| @@ -5,9 +5,26 @@ use std::str::FromStr; | |||
|
|
|||
| use http::uri::Authority; | |||
|
|
|||
| /// A fully qualified authority according to Kubernetes service naming | |||
| /// conventions. | |||
There was a problem hiding this comment.
Actually, this isn't restricted to things that are named according to Kuberentes conventions. I suggest something like "A normalized Authority."
| #[derive(Clone, Debug, Eq, Hash, PartialEq)] | ||
| pub struct FullyQualifiedAuthority(Authority); | ||
|
|
||
| /// A fully qualified authority, and whether or not the `Destination` service | ||
| /// should be queried for that name. |
There was a problem hiding this comment.
This comment just repeats exactly what the code says, so I don't think we should include it, so that we don't have to keep it in sync with code changes in the future.
| impl NamedAddress { | ||
| #[inline] | ||
| pub fn without_trailing_dot(&self) -> &Authority { | ||
| self.name.without_trailing_dot() |
There was a problem hiding this comment.
let's just drop this and have callers use self.name.without_trailing_dot() directly. In general we should avoid these forwarding methods unless they save a non-trivial effort on the caller side.
hawkw
dismissed
briansmith’s stale review
All requested changes made in 86a641a
|
I'm not sure why this PR's CI build has been in the "Waiting for status to be reported" state for over an hour --- I suspect Travis is misbehaving? |
I've had the same issue before:
(There are probably typos there, but you get the idea.) |
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
|
Thanks Brian, hopefully it will behave this time... |
|
I restarted the most recent build of this branch, as the failure was on the web build job. Since there were no web changes, I suspect the failure was spurious. |
#476) As requested by @briansmith in #415 (comment) and #415 (comment), I've refactored `FullyQualifiedAuthority::normalize` to _always_ return a `FullyQualifiedAuthority`, along with a boolean value indicating whether or not the Destination service should be used for that authority. This is in contrast to returning an `Option<FullyQualifiedAuthority>` where `None` indicated that the Destination service should not be used, which is what this function did previously. This is required for further progress on #415. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
linkerd#476) As requested by @briansmith in linkerd#415 (comment) and linkerd#415 (comment), I've refactored `FullyQualifiedAuthority::normalize` to _always_ return a `FullyQualifiedAuthority`, along with a boolean value indicating whether or not the Destination service should be used for that authority. This is in contrast to returning an `Option<FullyQualifiedAuthority>` where `None` indicated that the Destination service should not be used, which is what this function did previously. This is required for further progress on linkerd#415. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
As requested by @briansmith in #415 (comment) and #415 (comment), I've refactored
FullyQualifiedAuthority::normalizeto always return aFullyQualifiedAuthority, along with a boolean value indicating whether or not the Destination service should be used for that authority.This is in contrast to returning an
Option<FullyQualifiedAuthority>whereNoneindicated that the Destination service should not be used, which is what this function did previously.This is required for further progress on #415.