-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: report downstream check task results #1385
Changes from 2 commits
cbc53a1
9d8d088
6bfd3ad
fcdec1d
4b19264
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,8 @@ use crate::RoverClientError; | |
|
||
use graphql_client::*; | ||
|
||
use self::graph_check_workflow_query::CheckWorkflowStatus; | ||
use self::graph_check_workflow_query::GraphCheckWorkflowQueryGraphCheckWorkflowTasks::OperationsCheckTask; | ||
use self::graph_check_workflow_query::{CheckWorkflowStatus, CheckWorkflowTaskStatus}; | ||
|
||
use super::types::OperationsResult; | ||
|
||
|
@@ -69,14 +69,15 @@ fn get_check_response_from_data( | |
graph_ref: graph_ref.clone(), | ||
})?; | ||
|
||
let status = check_workflow.status.into(); | ||
let workflow_status = check_workflow.status; | ||
let mut operations_status = None; | ||
let mut operations_target_url = None; | ||
let mut operations_result: Option<OperationsResult> = None; | ||
let mut target_url = None; | ||
let mut number_of_checked_operations: u64 = 0; | ||
let mut changes = Vec::new(); | ||
for task in check_workflow.tasks { | ||
if let OperationsCheckTask(task) = task { | ||
target_url = task.target_url; | ||
operations_status = Some(task.status); | ||
operations_target_url = task.target_url; | ||
if let Some(result) = task.result { | ||
number_of_checked_operations = | ||
result.number_of_checked_operations.try_into().unwrap(); | ||
|
@@ -85,32 +86,48 @@ fn get_check_response_from_data( | |
} | ||
} | ||
|
||
if let Some(result) = operations_result { | ||
if matches!(operations_status, Some(CheckWorkflowTaskStatus::FAILED)) | ||
|| matches!(workflow_status, CheckWorkflowStatus::PASSED) | ||
{ | ||
let result = operations_result.ok_or(RoverClientError::AdhocError { | ||
msg: "Operations check task has no result.".to_string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a better error we can return here/do we think it is unreachable/other changes here make this unlikely? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a check workflow has passed, then all its tasks must have passed, which includes the operations check task (so it must have a result). Similarly, if the operations check task has failed, then it also must have a result. So if we encounter this message/case, it basically means there's a bug in our API, or the semantics of our task API have changed in a breaking way. (So at the very least, it should be unlikely.) What error type would you recommend throwing in this case? (Could go with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go with the |
||
})?; | ||
let mut changes = Vec::with_capacity(result.changes.len()); | ||
for change in result.changes { | ||
changes.push(SchemaChange { | ||
code: change.code, | ||
severity: change.severity.into(), | ||
description: change.description, | ||
}); | ||
} | ||
} | ||
|
||
// The `graph` check response does not return this field | ||
// only `subgraph` check does. Since `CheckResponse` is shared | ||
// between `graph` and `subgraph` checks, defaulting this | ||
// to false for now since its currently only used in | ||
// `check_response.rs` to format better console messages. | ||
let core_schema_modified = false; | ||
// The `graph` check response does not return this field | ||
// only `subgraph` check does. Since `CheckResponse` is shared | ||
// between `graph` and `subgraph` checks, defaulting this | ||
// to false for now since its currently only used in | ||
// `check_response.rs` to format better console messages. | ||
let core_schema_modified = false; | ||
|
||
CheckResponse::try_new( | ||
target_url, | ||
number_of_checked_operations, | ||
changes, | ||
status, | ||
graph_ref, | ||
false, | ||
core_schema_modified, | ||
) | ||
CheckResponse::try_new( | ||
operations_target_url, | ||
number_of_checked_operations, | ||
changes, | ||
workflow_status.into(), | ||
graph_ref, | ||
core_schema_modified, | ||
) | ||
} else { | ||
// Note that graph IDs and variants don't need percent-encoding due to their regex restrictions. | ||
let default_target_url = format!( | ||
"https://studio.apollographql.com/graph/{}/checks?variant={}", | ||
graph_ref.name, graph_ref.variant | ||
); | ||
Err(RoverClientError::OtherCheckTaskFailure { | ||
has_build_task: false, | ||
has_downstream_task: false, | ||
target_url: operations_target_url.unwrap_or(default_target_url), | ||
}) | ||
} | ||
} | ||
|
||
fn get_target_url_from_data(data: QueryResponseData) -> Option<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.
i wonder if this is something we could express in the schema? perhaps downstream tasks are nested under build task results?
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.
We could potentially nest dependent tasks under their dependencies, although it becomes a bit trickier if dependents have multiple dependencies/the task graph gets more complicated. (It would be nice to represent the relationship in some programmatic fashion though in the API.)