more versioned tests (#5)

Found a couple of bugs:

1. We were running git in the current directory rather than the repo
root (whoops).
2. The symlink resolution for wire-compatible changes was slightly
wrong.

Author: sunshowers

crates/dropshot-api-manager/src/cmd/check.rs

@@ -30,7 +30,8 @@ pub(crate) fn check_impl(
     let (local_files, errors) = env.local_source.load(apis, &styles)?;
     display_load_problems(&errors, &styles)?;
 
-    let (blessed, errors) = blessed_source.load(apis, &styles)?;
+    let (blessed, errors) =
+        blessed_source.load(&env.repo_root, apis, &styles)?;
     display_load_problems(&errors, &styles)?;
 
     let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files);

crates/dropshot-api-manager/src/cmd/debug.rs

@@ -30,7 +30,8 @@ pub(crate) fn debug_impl(
     dump_structure(&local_files, &errors);
 
     // Print information about what we found in Git.
-    let (blessed, errors) = blessed_source.load(apis, &styles)?;
+    let (blessed, errors) =
+        blessed_source.load(&env.repo_root, apis, &styles)?;
     dump_structure(&blessed, &errors);
 
     // Print information about generated files.

crates/dropshot-api-manager/src/cmd/generate.rs

@@ -49,7 +49,8 @@ pub(crate) fn generate_impl(
     let (local_files, errors) = env.local_source.load(apis, &styles)?;
     display_load_problems(&errors, &styles)?;
 
-    let (blessed, errors) = blessed_source.load(apis, &styles)?;
+    let (blessed, errors) =
+        blessed_source.load(&env.repo_root, apis, &styles)?;
     display_load_problems(&errors, &styles)?;
 
     let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files);

crates/dropshot-api-manager/src/environment.rs

@@ -202,6 +202,7 @@ impl BlessedSource {
     /// Load the blessed OpenAPI documents
     pub fn load(
         &self,
+        repo_root: &Utf8Path,
         apis: &ManagedApis,
         styles: &Styles,
     ) -> anyhow::Result<(BlessedFiles, ErrorAccumulator)> {
@@ -227,6 +228,7 @@ impl BlessedSource {
                 );
                 Ok((
                     BlessedFiles::load_from_git_parent_branch(
+                        repo_root,
                         revision,
                         directory,
                         apis,

crates/dropshot-api-manager/src/git.rs

@@ -20,9 +20,10 @@ NewtypeFrom! { () pub struct GitRevision(String); }
 
 /// Given a revision, return its merge base with HEAD
 pub fn git_merge_base_head(
+    repo_root: &Utf8Path,
     revision: &GitRevision,
 ) -> anyhow::Result<GitRevision> {
-    let mut cmd = git_start();
+    let mut cmd = git_start(repo_root);
     cmd.arg("merge-base").arg("--all").arg("HEAD").arg(revision.as_str());
     let label = cmd_label(&cmd);
     let stdout = do_run(&mut cmd)?;
@@ -39,10 +40,11 @@ pub fn git_merge_base_head(
 
 /// List files recursively under some path `path` in Git revision `revision`.
 pub fn git_ls_tree(
+    repo_root: &Utf8Path,
     revision: &GitRevision,
     directory: &Utf8Path,
 ) -> anyhow::Result<Vec<Utf8PathBuf>> {
-    let mut cmd = git_start();
+    let mut cmd = git_start(repo_root);
     cmd.arg("ls-tree")
         .arg("-r")
         .arg("-z")
@@ -75,19 +77,22 @@ pub fn git_ls_tree(
 /// Returns the contents of the file at the given path `path` in Git revision
 /// `revision`.
 pub fn git_show_file(
+    repo_root: &Utf8Path,
     revision: &GitRevision,
     path: &Utf8Path,
 ) -> anyhow::Result<Vec<u8>> {
-    let mut cmd = git_start();
+    let mut cmd = git_start(repo_root);
     cmd.arg("cat-file").arg("blob").arg(format!("{}:{}", revision, path));
     let stdout = do_run(&mut cmd)?;
     Ok(stdout.into_bytes())
 }
 
 /// Begin assembling an invocation of git(1)
-fn git_start() -> Command {
+fn git_start(repo_root: &Utf8Path) -> Command {
     let git = std::env::var("GIT").ok().unwrap_or_else(|| String::from("git"));
-    Command::new(&git)
+    let mut command = Command::new(&git);
+    command.current_dir(repo_root);
+    command
 }
 
 /// Runs an assembled git(1) command, returning stdout on success and an error

crates/dropshot-api-manager/src/resolved.rs

@@ -679,37 +679,82 @@ fn resolve_api<'a>(
                 } else {
                     // latest_local is different from latest_generated.
                     //
-                    // We never want to update blessed documents. But
-                    // latest_generated might have wire-compatible changes which
-                    // would cause the hash to change.
+                    // We never want to update the local copies of blessed
+                    // documents. But latest_generated might have
+                    // wire-compatible (trivial) changes which would cause the
+                    // hash to change, so we need to handle this case with care.
                     //
-                    // There are two possibilities:
+                    // The possibilities are:
                     //
-                    // 1. latest_local is blessed and latest_generated has
-                    //    wire-compatible changes. In that case, we don't update
-                    //    the symlink.
-                    // 2. latest_local is blessed and latest_generated has
+                    // 1. latest_local is blessed, latest_generated has the same
+                    //    version as latest_local, and it has wire-compatible
+                    //    changes. In that case, don't update the symlink.
+                    // 2. latest_local is blessed, latest_generated has the same
+                    //    version as latest_local, and latest_generated has
                     //    wire-*incompatible* changes. In that case, we'd have
                     //    returned errors in the by_version map above, and we
                     //    wouldn't want to update the symlink in any case.
-                    // 3. latest_local is not blessed. In that case, we do
+                    // 3. latest_local is blessed, and latest_generated is
+                    //    blessed but a *different* version. This means that
+                    //    the latest version was retired. In this case,
+                    //    we want to update the symlink to the blessed hash
+                    //    corresponding to the latest generated version.
+                    // 4. latest_local is not blessed. In that case, we do
                     //    want to update the symlink.
-                    //
-                    // This boils down to simply checking if latest_generated is
-                    // a blessed version.
-                    let version = latest_generated
+                    let generated_version = latest_generated
+                        .version()
+                        .expect("versioned APIs have a version");
+                    let local_version = latest_local
                         .version()
                         .expect("versioned APIs have a version");
-                    if let Some(resolution) = by_version.get(version) {
+                    if let Some(resolution) = by_version.get(generated_version)
+                    {
                         match resolution.kind() {
                             ResolutionKind::Lockstep => {
                                 unreachable!("this is a versioned API");
                             }
-                            ResolutionKind::Blessed => {
-                                // latest_generated is blessed, so don't update
-                                // the symlink.
+                            // Case 1 and 2 above.
+                            ResolutionKind::Blessed
+                                if generated_version == local_version =>
+                            {
+                                // latest_generated is blessed and the same
+                                // version as latest_local, so don't update the
+                                // symlink.
                                 None
                             }
+                            ResolutionKind::Blessed => {
+                                // latest_generated is blessed, and has a
+                                // different version from latest_local. In this
+                                // case, we want to update the symlink to the
+                                // blessed version matching latest_generated
+                                // (not latest_generated, in case it's different
+                                // from the blessed version in a wire-compatible
+                                // way!)
+                                let api_blessed =
+                                    api_blessed.unwrap_or_else(|| {
+                                        panic!(
+                                            "for {}, Blessed means \
+                                             api_blessed exists",
+                                            api.ident()
+                                        )
+                                    });
+                                let blessed = api_blessed
+                                    .versions()
+                                    .get(generated_version)
+                                    .unwrap_or_else(|| {
+                                        panic!(
+                                            "for {} v{}, Blessed means \
+                                             generated_version exists",
+                                            api.ident(),
+                                            generated_version
+                                        );
+                                    });
+                                Some(Problem::LatestLinkStale {
+                                    api_ident: api.ident().clone(),
+                                    link: blessed.spec_file_name(),
+                                    found: latest_local,
+                                })
+                            }
                             ResolutionKind::NewLocally => {
                                 // latest_generated is not blessed, so update
                                 // the symlink.

crates/dropshot-api-manager/src/spec_files_blessed.rs

@@ -95,13 +95,15 @@ impl BlessedFiles {
     /// `M1` for blessed documents because you haven't yet merged in commits M2,
     /// M3, and M4.
     pub fn load_from_git_parent_branch(
+        repo_root: &Utf8Path,
         branch: &GitRevision,
         directory: &Utf8Path,
         apis: &ManagedApis,
         error_accumulator: &mut ErrorAccumulator,
     ) -> anyhow::Result<BlessedFiles> {
-        let revision = git_merge_base_head(branch)?;
+        let revision = git_merge_base_head(repo_root, branch)?;
         Self::load_from_git_revision(
+            repo_root,
             &revision,
             directory,
             apis,
@@ -111,14 +113,15 @@ impl BlessedFiles {
 
     /// Load OpenAPI documents from the given Git revision and directory.
     pub fn load_from_git_revision(
+        repo_root: &Utf8Path,
         commit: &GitRevision,
         directory: &Utf8Path,
         apis: &ManagedApis,
         error_accumulator: &mut ErrorAccumulator,
     ) -> anyhow::Result<BlessedFiles> {
         let mut api_files: ApiSpecFilesBuilder<BlessedApiSpecFile> =
             ApiSpecFilesBuilder::new(apis, error_accumulator);
-        let files_found = git_ls_tree(commit, directory)?;
+        let files_found = git_ls_tree(repo_root, commit, directory)?;
         for f in files_found {
             // We should be looking at either a single-component path
             // ("api.json") or a file inside one level of directory hierarchy
@@ -134,7 +137,8 @@ impl BlessedFiles {
 
             // Read the contents. Use "/" rather than "\" on Windows.
             let file_name = format!("{directory}/{f}");
-            let contents = git_show_file(commit, file_name.as_ref())?;
+            let contents =
+                git_show_file(repo_root, commit, file_name.as_ref())?;
             if parts.len() == 1 {
                 if let Some(file_name) = api_files.lockstep_file_name(parts[0])
                 {

crates/integration-tests/src/common/fixtures.rs

@@ -451,3 +451,43 @@ pub mod versioned_user {
         pub permissions: Vec<String>,
     }
 }
+
+/// Reduced versioned health API for testing version removal scenarios.
+pub mod versioned_health_reduced {
+    use super::*;
+    use dropshot_api_manager_types::api_versions;
+
+    api_versions!([(2, WITH_DETAILED_STATUS), (1, INITIAL),]);
+
+    #[dropshot::api_description]
+    pub trait VersionedHealthApi {
+        type Context;
+
+        /// Check if the service is healthy (all versions).
+        #[endpoint {
+            method = GET,
+            path = "/health",
+            operation_id = "health_check",
+            versions = "1.0.0"..
+        }]
+        async fn health_check(
+            rqctx: RequestContext<Self::Context>,
+        ) -> Result<HttpResponseOk<HealthStatusV1>, HttpError>;
+
+        /// Get detailed health status (v2+).
+        #[endpoint {
+            method = GET,
+            path = "/health/detailed",
+            operation_id = "detailed_health_check",
+            versions = "2.0.0"..
+        }]
+        async fn detailed_health_check(
+            rqctx: RequestContext<Self::Context>,
+        ) -> Result<HttpResponseOk<DetailedHealthStatus>, HttpError>;
+    }
+
+    // Reuse the same response types from the main versioned_health module.
+    pub use super::versioned_health::{
+        DependencyStatus, DetailedHealthStatus, HealthStatusV1,
+    };
+}

crates/integration-tests/src/common/mod.rs

@@ -43,7 +43,10 @@ impl TestEnvironment {
         let documents_dir = workspace_root.child("documents");
 
         // Initialize git repository in workspace root.
-        Self::run_git_command(&workspace_root, &["init"])?;
+        Self::run_git_command(
+            &workspace_root,
+            &["init", "--initial-branch", "main"],
+        )?;
         Self::run_git_command(
             &workspace_root,
             &["config", "user.name", "Test User"],
@@ -70,7 +73,11 @@ impl TestEnvironment {
             "test-openapi-manager",
             workspace_root.as_path(),
             "documents",
-        )?;
+        )?
+        // Use "main" rather than the default "origin/main" since we're not
+        // pushing to an upstream. A commit to main automatically marks the
+        // document blessed.
+        .with_default_git_branch("main");
 
         Ok(Self { temp_dir, workspace_root, documents_dir, environment })
     }
@@ -110,6 +117,15 @@ impl TestEnvironment {
             .with_context(|| format!("failed to read file: {}", path))
     }
 
+    pub fn read_link(
+        &self,
+        relative_path: impl AsRef<Utf8Path>,
+    ) -> Result<Utf8PathBuf> {
+        let path = self.workspace_root.join(relative_path);
+        path.read_link_utf8()
+            .with_context(|| format!("failed to read link: {}", path))
+    }
+
     /// Check if a file exists in the workspace.
     pub fn file_exists(&self, relative_path: impl AsRef<Utf8Path>) -> bool {
         self.workspace_root.join(relative_path.as_ref()).exists()
@@ -199,10 +215,13 @@ impl TestEnvironment {
         &self,
         api_ident: &str,
     ) -> Result<String> {
-        self.read_file(format!(
-            "documents/{}/{}-latest.json",
-            api_ident, api_ident
-        ))
+        // Try reading the link to ensure it's a symlink.
+        let file_name =
+            format!("documents/{}/{}-latest.json", api_ident, api_ident);
+        let target = self.read_link(&file_name)?;
+        eprintln!("** symlink target: {}", target);
+
+        self.read_file(&file_name)
     }
 
     /// Commit documents to git (for blessed document workflow testing).
@@ -456,3 +475,42 @@ pub fn create_mixed_test_apis() -> Result<ManagedApis> {
     ];
     ManagedApis::new(configs).context("failed to create mixed ManagedApis")
 }
+
+/// Create versioned health API with a trivial change (title/metadata updated).
+pub fn create_versioned_health_test_apis_with_trivial_change()
+-> Result<ManagedApis> {
+    // Create a modified API config that would produce different OpenAPI documents.
+    let mut config = versioned_health_test_api();
+
+    // Modify the title to create a different document signature.
+    config.title = "Modified Versioned Health API";
+    config.metadata.description =
+        Some("A versioned health API with breaking changes");
+
+    ManagedApis::new(vec![config])
+        .context("failed to create trivial change versioned health ManagedApis")
+}
+
+/// Create versioned health API with reduced versions (simulating version removal).
+pub fn create_versioned_health_test_apis_reduced_versions()
+-> Result<ManagedApis> {
+    // Create a configuration similar to versioned health but with fewer versions.
+    // We'll create a new fixture for this.
+    let config = ManagedApiConfig {
+        ident: "versioned-health",
+        versions: Versions::Versioned {
+            // Use a subset of versions (only 1.0.0 and 2.0.0, not 3.0.0).
+            supported_versions: fixtures::versioned_health_reduced::supported_versions(),
+        },
+        title: "Versioned Health API",
+        metadata: ManagedApiMetadata {
+            description: Some("A versioned health API with reduced versions"),
+            ..Default::default()
+        },
+        api_description: fixtures::versioned_health_reduced::versioned_health_api_mod::stub_api_description,
+        extra_validation: None,
+    };
+
+    ManagedApis::new(vec![config])
+        .context("failed to create reduced versioned health ManagedApis")
+}