Skip to content

Bail early on bad token #482

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Bail early on bad token #482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion cli-tests/src/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,8 +834,80 @@ async fn test_variant_propagation() {
let reader = BufReader::new(file);
let bundle_meta: BundleMeta = serde_json::from_reader(reader).unwrap();
assert_eq!(bundle_meta.variant, Some("test-variant".to_string()));
}

#[tokio::test(flavor = "multi_thread")]
async fn test_404_quarantine_config_skips_bundle_upload() {
let temp_dir = tempdir().unwrap();
generate_mock_git_repo(&temp_dir);
generate_mock_valid_junit_xmls(&temp_dir);

let mut mock_server_builder = MockServerBuilder::new();

// Mock getQuarantineConfig to return 404
mock_server_builder.set_get_quarantining_config_handler(
|Json(_): Json<GetQuarantineConfigRequest>| async {
Err::<Json<GetQuarantineConfigResponse>, StatusCode>(StatusCode::NOT_FOUND)
},
);

let state = mock_server_builder.spawn_mock_server().await;

let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone())
.command()
.assert()
.failure();

// Verify only the quarantine config request was made
let requests = state.requests.lock().unwrap().clone();
assert_eq!(requests.len(), 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we expecting 0 requests instead of 1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request didn't go through because it raises an error above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how do you get the error from the state server without sending a request to it? Doesn't the cli have to send a request to get the 404 from the server?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in the mock_server is such that if there's an error during the request then this will not be populated if there is an error. I agree it's confusing given the name and we should move towards it always being populated.


// HINT: View CLI output with `cargo test -- --nocapture`
println!("{assert}");
}

#[tokio::test(flavor = "multi_thread")]
async fn test_500_quarantine_config_proceeds_with_bundle_upload() {
let temp_dir = tempdir().unwrap();
generate_mock_git_repo(&temp_dir);
generate_mock_valid_junit_xmls(&temp_dir);

let mut mock_server_builder = MockServerBuilder::new();

// Mock getQuarantineConfig to return 500
mock_server_builder.set_get_quarantining_config_handler(
|Json(_): Json<GetQuarantineConfigRequest>| async {
Err::<Json<GetQuarantineConfigResponse>, StatusCode>(StatusCode::INTERNAL_SERVER_ERROR)
},
);

let state = mock_server_builder.spawn_mock_server().await;

let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone())
.command()
.assert()
.success();

// Verify all expected requests were made in order
let requests = state.requests.lock().unwrap().clone();
assert_eq!(requests.len(), 3);
let mut requests_iter = requests.into_iter();

// quarantine config failed, so first request should be create bundle upload
let upload_request = assert_matches!(requests_iter.next().unwrap(), RequestPayload::CreateBundleUpload(ur) => ur);
assert_eq!(
upload_request.repo,
Repo {
host: String::from("github.com"),
owner: String::from("trunk-io"),
name: String::from("analytics-cli"),
}
);

// second request should be s3 upload
assert_matches!(requests_iter.next().unwrap(), RequestPayload::S3Upload(_));

// Fourth request should be telemetry
// third request should be telemetry
assert_matches!(
requests_iter.next().unwrap(),
RequestPayload::TelemetryUploadMetrics(_)
Expand Down
6 changes: 3 additions & 3 deletions cli/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context(
api_client: &ApiClient,
file_set_builder: &FileSetBuilder,
test_run_result: &Option<TestRunResult>,
) -> i32 {
) -> anyhow::Result<i32> {
// Run the quarantine step and update the exit code.
let failed_tests_extractor = FailedTestsExtractor::new(
&meta.base_props.repo.repo,
Expand Down Expand Up @@ -324,12 +324,12 @@ pub async fn gather_exit_code_and_quarantined_tests_context(
Some(failed_tests_extractor),
test_run_result.as_ref().map(|t| t.exit_code),
)
.await
.await?
};

meta.base_props.quarantined_tests = quarantined_tests;

exit_code
Ok(exit_code)
}

pub async fn gather_upload_id_context(
Expand Down
47 changes: 27 additions & 20 deletions cli/src/context_quarantine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use context::{
};
use prost::Message;

use crate::error_report::{log_error, Context};
use crate::error_report::{is_user_error, log_error, Context};

#[derive(Debug, Default, Clone)]
pub struct QuarantineContext {
Expand Down Expand Up @@ -211,7 +211,7 @@ pub async fn gather_quarantine_context(
file_set_builder: &FileSetBuilder,
failed_tests_extractor: Option<FailedTestsExtractor>,
test_run_exit_code: Option<i32>,
) -> QuarantineContext {
) -> anyhow::Result<QuarantineContext> {
let failed_tests_extractor = failed_tests_extractor.unwrap_or_else(|| {
FailedTestsExtractor::new(
&request.repo,
Expand All @@ -224,10 +224,10 @@ pub async fn gather_quarantine_context(

if file_set_builder.no_files_found() {
tracing::info!("No test output files found, not quarantining any tests.");
return QuarantineContext {
return Ok(QuarantineContext {
exit_code,
..Default::default()
};
});
}

let quarantine_config = if !failed_tests_extractor.failed_tests().is_empty()
Expand All @@ -238,19 +238,26 @@ pub async fn gather_quarantine_context(
.any(|file_set| file_set.file_set_type == FileSetType::Junit)
{
tracing::info!("Checking if failed tests can be quarantined");
let result = api_client.get_quarantining_config(request).await;

if let Err(ref err) = result {
log_error(
err,
Context {
base_message: Some("Unable to find quarantine config".into()),
org_url_slug: request.org_url_slug.clone(),
},
);
match api_client.get_quarantining_config(request).await {
Ok(response) => response,
Err(err) => {
log_error(
&err,
Context {
base_message: Some("Unable to find quarantine config".into()),
org_url_slug: request.org_url_slug.clone(),
},
);
if is_user_error(&err) {
Err(err)?
} else {
return Ok(QuarantineContext {
exit_code,
..Default::default()
});
}
}
}

result.unwrap_or_default()
} else {
tracing::debug!("Skipping quarantine check.");
api::message::GetQuarantineConfigResponse::default()
Expand All @@ -259,10 +266,10 @@ pub async fn gather_quarantine_context(
// if quarantining is not enabled, return exit code and empty quarantine status
if quarantine_config.is_disabled {
tracing::info!("Quarantining is not enabled, not quarantining any tests");
return QuarantineContext {
return Ok(QuarantineContext {
exit_code,
quarantine_status: QuarantineBulkTestStatus::default(),
};
});
} else {
// quarantining is enabled, continue with quarantine process and update exit code
exit_code = test_run_exit_code.unwrap_or_else(|| failed_tests_extractor.exit_code());
Expand Down Expand Up @@ -348,10 +355,10 @@ pub async fn gather_quarantine_context(
};
tracing::info!("");

QuarantineContext {
Ok(QuarantineContext {
exit_code,
quarantine_status: quarantine_results,
}
})
}

fn log_failure(
Expand Down
39 changes: 22 additions & 17 deletions cli/src/error_report.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ pub struct Context {
pub org_url_slug: String,
}

pub fn is_user_error(request_err: &anyhow::Error) -> bool {
let root_cause = request_err.root_cause();
if let Some(reqwest_error) = root_cause.downcast_ref::<reqwest::Error>() {
if let Some(status) = reqwest_error.status() {
return status == StatusCode::UNAUTHORIZED
|| status == StatusCode::FORBIDDEN
|| status == StatusCode::NOT_FOUND;
}
}
false
}

pub fn log_error(error: &anyhow::Error, context: Context) -> i32 {
let root_cause = error.root_cause();
let Context {
Expand All @@ -50,23 +62,16 @@ pub fn log_error(error: &anyhow::Error, context: Context) -> i32 {
}

let api_host = get_api_host();
if let Some(reqwest_error) = root_cause.downcast_ref::<reqwest::Error>() {
if let Some(status) = reqwest_error.status() {
if status == StatusCode::UNAUTHORIZED
|| status == StatusCode::FORBIDDEN
|| status == StatusCode::NOT_FOUND
{
tracing::warn!(
"{}",
add_settings_url_to_context(
message(base_message, UNAUTHORIZED_CONTEXT),
Some(api_host),
&org_url_slug
)
);
return exitcode::SOFTWARE;
}
}
if is_user_error(error) {
tracing::warn!(
"{}",
add_settings_url_to_context(
message(base_message, UNAUTHORIZED_CONTEXT),
Some(api_host),
&org_url_slug
)
);
return exitcode::SOFTWARE;
}

if let Some(base_message) = base_message {
Expand Down
2 changes: 1 addition & 1 deletion cli/src/upload_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ pub async fn run_upload(
&file_set_builder,
&test_run_result,
)
.await;
.await?;

let upload_started_at = chrono::Utc::now();
tracing::info!("Uploading test results...");
Expand Down