-
Notifications
You must be signed in to change notification settings - Fork 109
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
Added Service as a new target type #3022
base: main
Are you sure you want to change the base?
Added Service as a new target type #3022
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of doc nitpicks etc.
@@ -87,42 +94,67 @@ fn make_simple_target_custom_schema(gen: &mut SchemaGenerator) -> schemars::sche | |||
/// } | |||
/// ``` | |||
/// | |||
/// Setup above will result in a session targeting the `bear-pod` Kubernetes pod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Setup above will result in a session targeting the `bear-pod` Kubernetes pod | |
/// The setup above will result in a session targeting the `bear-pod` Kubernetes pod |
@@ -87,42 +94,67 @@ fn make_simple_target_custom_schema(gen: &mut SchemaGenerator) -> schemars::sche | |||
/// } | |||
/// ``` | |||
/// | |||
/// Setup above will result in a session targeting the `bear-pod` Kubernetes pod | |||
/// in the user's default namespace. Target container will be chosen by mirrord. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// in the user's default namespace. Target container will be chosen by mirrord. | |
/// in the user's default namespace. A target container will be chosen by mirrord. |
/// } | ||
/// ``` | ||
/// | ||
/// Setup above will result in a session targeting the `bear-pod-container` container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Setup above will result in a session targeting the `bear-pod-container` container | |
/// The setup above will result in a session targeting the `bear-pod-container` container |
/// } | ||
/// } | ||
/// ``` | ||
/// | ||
/// Setup above will result in a session targeting the `bear-pod-container` container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Setup above will result in a session targeting the `bear-pod-container` container | |
/// The setup above will result in a session targeting the `bear-pod-container` container |
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub path: Option<Target>, | ||
|
||
/// ### target.namespace {#target-namespace} | ||
/// | ||
/// Namespace where the target lives. | ||
/// | ||
/// Defaults to `"default"`. | ||
/// Defaults to Kubernetes user's default namespace (defined in Kubernetes context). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Defaults to Kubernetes user's default namespace (defined in Kubernetes context). | |
/// Defaults to the Kubernetes user's default namespace (defined in Kubernetes context). |
Pod(pod::PodTarget), | ||
|
||
/// <!--${internal}--> | ||
/// Mirror a rollout. | ||
/// [Rollout](https://argoproj.github.io/argo-rollouts/#how-does-it-work). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// [Rollout](https://argoproj.github.io/argo-rollouts/#how-does-it-work). | |
/// [Argo Rollout](https://argoproj.github.io/argo-rollouts/#how-does-it-work). |
#[allow(async_fn_in_trait)] | ||
async fn runtime_data(&self, client: &Client, namespace: Option<&str>) -> Result<RuntimeData>; | ||
fn runtime_data( | ||
&self, | ||
client: &Client, | ||
namespace: Option<&str>, | ||
) -> impl Future<Output = Result<RuntimeData>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it better this way?
async fn get_selector_match_labels( | ||
resource: &Self::Resource, | ||
) -> Result<BTreeMap<String, String>> { | ||
fn get_selector_match_labels(resource: &Self::Resource) -> Result<BTreeMap<String, String>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should enable clippy's unused_async
lint? I'm surprised it's not on by default.
@@ -303,13 +276,18 @@ impl ResolvedTarget<false> { | |||
Ok(target) | |||
} | |||
|
|||
/// Check if the target can be used as a mirrord target. | |||
/// Checks if the target can be used as a via the mirrord Operator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence needs fixing. ("... as a via ...")
.target.namespace
doc #3009