Respond to review comments

Author: arlosi

.env.sample

@@ -34,12 +34,12 @@ export TEST_DATABASE_URL=
 # export S3_REGION=
 
 # Credentials for uploading index metadata to S3. You can leave these commented
-# out if you're not publishing to s3 from your crates.io instance.
-# export S3_INDEX_BUCKET
-# export S3_INDEX_ACCESS_KEY
-# export S3_INDEX_SECRET_KEY
+# out if you're not publishing index metadata to s3 from your crates.io instance.
+# export S3_INDEX_BUCKET=
+# export S3_INDEX_ACCESS_KEY=
+# export S3_INDEX_SECRET_KEY=
 # not needed if the S3 bucket is in US standard
-# export S3_INDEX_REGION
+# export S3_INDEX_REGION=
 
 # Upstream location of the registry index. Background jobs will push to
 # this URL. The default points to a local index for development.

cargo-registry-index/lib.rs

@@ -317,7 +317,7 @@ impl Repository {
     /// the given crate name for usage in URLs (i.e. with `/` separator).
     ///
     /// see <https://doc.rust-lang.org/cargo/reference/registries.html#index-format>
-    pub fn relative_index_file_as_string(name: &str) -> String {
+    pub fn relative_index_file_for_url(name: &str) -> String {
         let name = name.to_lowercase();
         Self::relative_index_file_helper(&name).join("/")
     }
@@ -358,7 +358,8 @@ impl Repository {
         self.push("refs/heads/master")
     }
 
-    /// Gets a list of files that have been modified since a given `starting_commit` (use `starting_commit = None` for a list of all files).
+    /// Gets a list of files that have been modified since a given `starting_commit`
+    /// (use `starting_commit = None` for a list of all files).
     pub fn get_files_modified_since(
         &self,
         starting_commit: Option<&str>,
@@ -385,12 +386,14 @@ impl Repository {
             .repository
             .find_commit(self.head_oid()?)?
             .as_object()
-            .peel_to_tree()?;
+            .peel_to_tree()
+            .context("failed to find tree for HEAD")?;
         let diff = self
             .repository
             .diff_tree_to_tree(starting_commit.as_ref(), Some(&head), None)
             .context("failed to run diff")?;
-        let files = diff.deltas()
+        let files = diff
+            .deltas()
             .map(|delta| delta.new_file())
             .filter(|file| file.exists())
             .map(|file| file.path().unwrap().to_path_buf())

src/admin/render_readmes.rs

@@ -2,7 +2,7 @@ use crate::{
     config, db,
     models::Version,
     schema::{crates, readme_renderings, versions},
-    uploaders::Uploader,
+    uploaders::{UploadBucket, Uploader},
 };
 use anyhow::{anyhow, Context};
 use std::{io::Read, path::Path, sync::Arc, thread};
@@ -14,6 +14,7 @@ use flate2::read::GzDecoder;
 use reqwest::{blocking::Client, header};
 use tar::{self, Archive};
 
+const CACHE_CONTROL_README: &str = "public,max-age=604800";
 const USER_AGENT: &str = "crates-admin";
 
 #[derive(clap::Parser, Debug)]
@@ -110,10 +111,26 @@ pub fn run(opts: Opts) -> anyhow::Result<()> {
             let handle = thread::spawn::<_, anyhow::Result<()>>(move || {
                 println!("[{}-{}] Rendering README...", krate_name, version.num);
                 let readme = get_readme(base_config.uploader(), &client, &version, &krate_name)?;
+                let content_length = readme.len() as u64;
+                let content = std::io::Cursor::new(readme);
+                let readme_path = format!("readmes/{0}/{0}-{1}.html", krate_name, version.num);
+                let mut extra_headers = header::HeaderMap::new();
+                extra_headers.insert(
+                    header::CACHE_CONTROL,
+                    header::HeaderValue::from_static(CACHE_CONTROL_README),
+                );
 
                 base_config
                     .uploader()
-                    .upload_readme(&client, &krate_name, &version.num, readme)
+                    .upload(
+                        &client,
+                        &readme_path,
+                        content,
+                        content_length,
+                        "text/html",
+                        extra_headers,
+                        UploadBucket::Default,
+                    )
                     .context("Failed to upload rendered README file to S3")?;
 
                 Ok(())

src/admin/upload_index.rs

@@ -1,3 +1,5 @@
+use std::time::{Duration, Instant};
+
 use crate::admin::dialoguer;
 use cargo_registry_index::{Repository, RepositoryConfig};
 use reqwest::blocking::Client;
@@ -31,7 +33,8 @@ pub fn run(opts: Opts) -> anyhow::Result<()> {
         return Ok(());
     }
 
-    for file in files {
+    let mut progress_update_time = Instant::now();
+    for (i, file) in files.iter().enumerate() {
         let crate_name = file.file_name().unwrap().to_str().unwrap();
         let path = repo.index_file(crate_name);
         if !path.exists() {
@@ -40,6 +43,13 @@ pub fn run(opts: Opts) -> anyhow::Result<()> {
         }
         let contents = std::fs::read_to_string(&path)?;
         uploader.upload_index(&client, crate_name, contents)?;
+
+        // Print a progress update every 10 seconds.
+        let now = Instant::now();
+        if now - progress_update_time > Duration::from_secs(10) {
+            progress_update_time = now;
+            println!("uploading {}/{}", i, files.len());
+        }
     }
 
     println!(

src/config/base.rs

@@ -83,15 +83,15 @@ impl Base {
                 // sniff/record it, but everywhere else we use https
                 "http",
             )),
-            index_bucket: Box::new(s3::Bucket::new(
+            index_bucket: Some(Box::new(s3::Bucket::new(
                 String::from("alexcrichton-test"),
                 None,
                 dotenv::var("S3_INDEX_ACCESS_KEY").unwrap_or_default(),
                 dotenv::var("S3_INDEX_SECRET_KEY").unwrap_or_default(),
                 // When testing we route all API traffic over HTTP so we can
                 // sniff/record it, but everywhere else we use https
                 "http",
-            )),
+            ))),
             cdn: None,
         };
         Self {
@@ -105,6 +105,16 @@ impl Base {
     }
 
     fn s3_panic_if_missing_keys() -> Uploader {
+        let index_bucket = match dotenv::var("S3_INDEX_BUCKET") {
+            Ok(name) => Some(Box::new(s3::Bucket::new(
+                name,
+                dotenv::var("S3_INDEX_REGION").ok(),
+                env("S3_INDEX_ACCESS_KEY"),
+                env("S3_INDEX_SECRET_KEY"),
+                "https",
+            ))),
+            Err(_) => None,
+        };
         Uploader::S3 {
             bucket: Box::new(s3::Bucket::new(
                 env("S3_BUCKET"),
@@ -113,18 +123,22 @@ impl Base {
                 env("S3_SECRET_KEY"),
                 "https",
             )),
-            index_bucket: Box::new(s3::Bucket::new(
-                env("S3_INDEX_BUCKET"),
-                dotenv::var("S3_INDEX_REGION").ok(),
-                env("S3_INDEX_ACCESS_KEY"),
-                env("S3_INDEX_SECRET_KEY"),
-                "https",
-            )),
+            index_bucket,
             cdn: dotenv::var("S3_CDN").ok(),
         }
     }
 
     fn s3_maybe_read_only() -> Uploader {
+        let index_bucket = match dotenv::var("S3_INDEX_BUCKET") {
+            Ok(name) => Some(Box::new(s3::Bucket::new(
+                name,
+                dotenv::var("S3_INDEX_REGION").ok(),
+                dotenv::var("S3_INDEX_ACCESS_KEY").unwrap_or_default(),
+                dotenv::var("S3_INDEX_SECRET_KEY").unwrap_or_default(),
+                "https",
+            ))),
+            Err(_) => None,
+        };
         Uploader::S3 {
             bucket: Box::new(s3::Bucket::new(
                 env("S3_BUCKET"),
@@ -133,13 +147,7 @@ impl Base {
                 dotenv::var("S3_SECRET_KEY").unwrap_or_default(),
                 "https",
             )),
-            index_bucket: Box::new(s3::Bucket::new(
-                env("S3_INDEX_BUCKET"),
-                dotenv::var("S3_INDEX_REGION").ok(),
-                dotenv::var("S3_INDEX_ACCESS_KEY").unwrap_or_default(),
-                dotenv::var("S3_INDEX_SECRET_KEY").unwrap_or_default(),
-                "https",
-            )),
+            index_bucket,
             cdn: dotenv::var("S3_CDN").ok(),
         }
     }

src/uploaders.rs

@@ -5,7 +5,7 @@ use crate::util::errors::{internal, AppResult};
 
 use std::env;
 use std::fs::{self, File};
-use std::io::{Cursor, SeekFrom};
+use std::io::{Cursor, Seek, SeekFrom};
 
 use crate::models::Crate;
 
@@ -19,7 +19,7 @@ pub enum Uploader {
     /// For test usage with `TestApp::with_proxy()`, the recording proxy is used.
     S3 {
         bucket: Box<s3::Bucket>,
-        index_bucket: Box<s3::Bucket>,
+        index_bucket: Option<Box<s3::Bucket>>,
         cdn: Option<String>,
     },
 
@@ -88,7 +88,7 @@ impl Uploader {
 
     /// Returns the internal path of an uploaded crate's index file.
     fn index_path(name: &str) -> String {
-        let path = cargo_registry_index::Repository::relative_index_file_as_string(name);
+        let path = cargo_registry_index::Repository::relative_index_file_for_url(name);
         format!("index/{}", path)
     }
 
@@ -100,11 +100,12 @@ impl Uploader {
     ///
     /// This function can panic on an `Self::Local` during development.
     /// Production and tests use `Self::S3` which should not panic.
-    pub fn upload<R: std::io::Read + std::io::Seek + Send + 'static>(
+    pub fn upload<R: std::io::Read + Send + 'static>(
         &self,
         client: &Client,
         path: &str,
         mut content: R,
+        content_length: u64,
         content_type: &str,
         extra_headers: header::HeaderMap,
         upload_bucket: UploadBucket,
@@ -116,20 +117,21 @@ impl Uploader {
                 ..
             } => {
                 let bucket = match upload_bucket {
-                    UploadBucket::Index => index_bucket,
-                    UploadBucket::Default => bucket,
+                    UploadBucket::Default => Some(bucket),
+                    UploadBucket::Index => index_bucket.as_ref(),
                 };
-                let content_length = content.seek(SeekFrom::End(0))?;
-                content.seek(SeekFrom::Start(0))?;
-
-                bucket.put(
-                    client,
-                    path,
-                    content,
-                    content_length,
-                    content_type,
-                    extra_headers,
-                )?;
+
+                if let Some(bucket) = bucket {
+                    bucket.put(
+                        client,
+                        path,
+                        content,
+                        content_length,
+                        content_type,
+                        extra_headers,
+                    )?;
+                }
+
                 Ok(Some(String::from(path)))
             }
             Uploader::Local => {
@@ -152,11 +154,13 @@ impl Uploader {
                 ..
             } => {
                 let bucket = match upload_bucket {
-                    UploadBucket::Index => index_bucket,
-                    UploadBucket::Default => bucket,
+                    UploadBucket::Default => Some(bucket),
+                    UploadBucket::Index => index_bucket.as_ref(),
                 };
 
-                bucket.delete(client, path)?;
+                if let Some(bucket) = bucket {
+                    bucket.delete(client, path)?;
+                }
             }
             Uploader::Local => {
                 let filename = env::current_dir().unwrap().join("local_uploads").join(path);
@@ -175,6 +179,7 @@ impl Uploader {
         vers: &semver::Version,
     ) -> AppResult<()> {
         let path = Uploader::crate_path(&krate.name, &vers.to_string());
+        let content_length = body.len() as u64;
         let content = Cursor::new(body);
         let mut extra_headers = header::HeaderMap::new();
         extra_headers.insert(
@@ -185,6 +190,7 @@ impl Uploader {
             http_client,
             &path,
             content,
+            content_length,
             "application/x-tar",
             extra_headers,
             UploadBucket::Default,
@@ -201,6 +207,7 @@ impl Uploader {
         readme: String,
     ) -> Result<()> {
         let path = Uploader::readme_path(crate_name, vers);
+        let content_length = readme.len() as u64;
         let content = Cursor::new(readme);
         let mut extra_headers = header::HeaderMap::new();
         extra_headers.insert(
@@ -211,6 +218,7 @@ impl Uploader {
             http_client,
             &path,
             content,
+            content_length,
             "text/html",
             extra_headers,
             UploadBucket::Default,
@@ -225,7 +233,9 @@ impl Uploader {
         index: String,
     ) -> Result<()> {
         let path = Uploader::index_path(crate_name);
-        let content = Cursor::new(index);
+        let mut content = Cursor::new(index);
+        let content_length = content.seek(SeekFrom::End(0))?;
+        content.seek(SeekFrom::Start(0))?;
         let mut extra_headers = header::HeaderMap::new();
         extra_headers.insert(
             header::CACHE_CONTROL,
@@ -235,6 +245,7 @@ impl Uploader {
             http_client,
             &path,
             content,
+            content_length,
             "text/plain",
             extra_headers,
             UploadBucket::Index,

src/worker/dump_db.rs

@@ -200,6 +200,7 @@ impl DumpTarball {
             &client,
             target_name,
             tarfile,
+            content_length,
             "application/gzip",
             header::HeaderMap::new(),
             UploadBucket::Default,