Skip to content
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

[TRUNK-12896] Include bundle_upload_id in meta.json on test bundle upload #105

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
8 changes: 4 additions & 4 deletions cli/src/clients.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::path::PathBuf;
use anyhow::Context;

use crate::types::{
BundleUploadLocation, CreateBundleUploadRequest, CreateRepoRequest,
CreateBundleUploadRequest, CreateBundleUploadResponse, CreateRepoRequest,
GetQuarantineBulkTestStatusRequest, QuarantineConfig, Repo,
};
use crate::utils::status_code_help;
Expand Down Expand Up @@ -48,12 +48,12 @@ pub async fn create_trunk_repo(
Ok(())
}

pub async fn get_bundle_upload_location(
pub async fn create_bundle_upload_intent(
origin: &str,
api_token: &str,
org_slug: &str,
repo: &Repo,
) -> anyhow::Result<Option<BundleUploadLocation>> {
) -> anyhow::Result<Option<CreateBundleUploadResponse>> {
let client = reqwest::Client::new();
let resp = match client
.post(format!("{}/v1/metrics/createBundleUpload", origin))
Expand All @@ -73,7 +73,7 @@ pub async fn get_bundle_upload_location(

if resp.status().is_success() {
return resp
.json::<Option<BundleUploadLocation>>()
.json::<Option<CreateBundleUploadResponse>>()
.await
.context("Failed to get response body as json");
}
Expand Down
27 changes: 15 additions & 12 deletions cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ use std::env;
use std::io::Write;
use std::time::{SystemTime, UNIX_EPOCH};

use anyhow::Context;
use clap::{Args, Parser, Subcommand};
use tokio_retry::strategy::ExponentialBackoff;
use tokio_retry::Retry;
use trunk_analytics_cli::bundler::BundlerUtil;
use trunk_analytics_cli::clients::{
create_trunk_repo, get_bundle_upload_location, put_bundle_to_s3,
create_bundle_upload_intent, create_trunk_repo, put_bundle_to_s3,
};
use trunk_analytics_cli::codeowners::CodeOwners;
use trunk_analytics_cli::constants::{
Expand Down Expand Up @@ -240,6 +241,14 @@ async fn run_upload(

let envs = EnvScanner::scan_env();
let os_info: String = env::consts::OS.to_string();

let upload_op = Retry::spawn(default_delay(), || {
create_bundle_upload_intent(&api_address, &token, &org_url_slug, &repo.repo)
})
.await?;

let upload = upload_op.context("Failed to create bundle upload.")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was looking for a way to set upload here to a CreateBundleUploadResponse from an Option<CreateBundleUploadResponse>, failing otherwise, since I don't think we'd want to continue if upload_op were None. LMK if that's an incorrect assumption. Based on this SO thread :) https://stackoverflow.com/questions/69738600/simplest-way-to-unwrap-an-option-and-return-error-if-none-anyhow

Copy link
Member

Choose a reason for hiding this comment

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

This is good, but I think we can do one better :)

In create_bundle_upload_intent I think we can and should change Option<CreateBundleUploadResponse> to just CreateBundleUploadResponse and then you won't need to do that here. That will also require changing from checking for is_success to checking if the API call is not successful (look at get_quarantining_config for example). This also makes create_bundle_upload_intent more consistent with our other API calls like get_quarantining_config. Eventually, I'll get around to cleaning this up so there's a better, more consistent pattern to all of the API calls with retries we make.

Removing the Option<T> type is a little auto-magical because it's how serde implicitly figures out how to deserialize the response using auto-implemented traits:
https://docs.rs/serde/latest/serde/de/index.html#implementations-of-deserialize-provided-by-serde

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! lmk how it looks


let meta = BundleMeta {
version: META_VERSION.to_string(),
cli_version: format!(
Expand All @@ -250,6 +259,7 @@ async fn run_upload(
),
org: org_url_slug.clone(),
repo: repo.clone(),
bundle_upload_id: upload.id,
tags,
file_sets,
envs,
Expand Down Expand Up @@ -287,22 +297,15 @@ async fn run_upload(
bundler.make_tarball(&bundle_time_file)?;
log::info!("Flushed temporary tarball to {:?}", bundle_time_file);

let upload_op = Retry::spawn(default_delay(), || {
get_bundle_upload_location(&api_address, &token, &org_url_slug, &repo.repo)
})
.await?;

if dry_run {
log::info!("Dry run, skipping upload.");
return Ok(exit_code);
}

if let Some(upload) = upload_op {
Retry::spawn(default_delay(), || {
put_bundle_to_s3(&upload.url, &bundle_time_file)
})
.await?;
}
Retry::spawn(default_delay(), || {
put_bundle_to_s3(&upload.url, &bundle_time_file)
})
.await?;

let remote_urls = vec![repo.repo_url.clone()];
Retry::spawn(default_delay(), || {
Expand Down
4 changes: 3 additions & 1 deletion cli/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ pub struct QuarantineConfig {
}

#[derive(Debug, Serialize, Clone, Deserialize)]
pub struct BundleUploadLocation {
pub struct CreateBundleUploadResponse {
pub id: String,
pub url: String,
pub key: String,
}
Expand Down Expand Up @@ -204,6 +205,7 @@ pub struct BundleMeta {
pub cli_version: String,
pub org: String,
pub repo: BundleRepo,
pub bundle_upload_id: String,
pub tags: Vec<CustomTag>,
pub file_sets: Vec<FileSet>,
pub envs: std::collections::HashMap<String, String>,
Expand Down
7 changes: 4 additions & 3 deletions cli/tests/test_utils/mock_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use tempfile::tempdir;
use tokio::net::TcpListener;
use tokio::spawn;
use trunk_analytics_cli::types::{
BundleUploadLocation, CreateBundleUploadRequest, CreateRepoRequest,
CreateBundleUploadRequest, CreateBundleUploadResponse, CreateRepoRequest,
GetQuarantineBulkTestStatusRequest, QuarantineConfig,
};

Expand Down Expand Up @@ -100,7 +100,7 @@ async fn repo_create_handler(
async fn create_bundle_handler(
State(state): State<SharedMockServerState>,
Json(create_bundle_upload_request): Json<CreateBundleUploadRequest>,
) -> Json<BundleUploadLocation> {
) -> Json<CreateBundleUploadResponse> {
state
.requests
.lock()
Expand All @@ -109,7 +109,8 @@ async fn create_bundle_handler(
create_bundle_upload_request,
));
let host = &state.host;
Json(BundleUploadLocation {
Json(CreateBundleUploadResponse {
id: String::from("test-bundle-upload-id"),
url: format!("{host}/s3upload"),
key: String::from("unused"),
})
Expand Down
1 change: 1 addition & 0 deletions cli/tests/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ async fn upload_bundle() {
bundle_meta.repo.repo_head_author_email,
"your.email@example.com"
);
assert_eq!(bundle_meta.bundle_upload_id, "test-bundle-upload-id");
assert_eq!(bundle_meta.tags, &[]);
assert_eq!(bundle_meta.file_sets.len(), 1);
assert_eq!(bundle_meta.envs.get("CI"), Some(&String::from("1")));
Expand Down