3. split out api, test_utils, and bundlerepo from cli #109

dfrankland · 2024-10-01T04:30:50Z

depends on #108

Primary purpose of this change is to split out BundleRepo from cli to context to add validation for the context we gather from git repos.

test_utils got split out from cli because BundleRepo tests rely on some of the utils there.

api got split out from cli because test_utils relies on structs used in mocking API calls.

trunk-staging-io · 2024-10-01T04:43:24Z

✅ 86 passed ⋅ (learn more)

_{settings ⋅ docs ⋅ learn more about trunk.io}

dfrankland · 2024-10-02T00:20:00Z

context/src/repo/mod.rs

Moved from CLI to this crate since we will want to use validate and use git details as a part of context we use for uploads.

dfrankland · 2024-10-02T00:21:43Z

context/src/repo/mod.rs

+        repo_head_sha: Option<String>,
+        repo_head_branch: Option<String>,
+        repo_head_commit_epoch: Option<String>,
+    ) -> anyhow::Result<BundleRepo> {
+        let mut bundle_repo_options = BundleRepoOptions {
+            repo_root: repo_root
+                .as_ref()
+                .map(|repo_root| PathBuf::from(repo_root))
+                .or_else(|| std::env::current_dir().ok()),
+            repo_url,
+            repo_head_sha,
+            repo_head_branch,
+            repo_head_commit_epoch: repo_head_commit_epoch.and_then(|s| s.parse().ok()),
+        };
+
+        let mut head_commit_message = None;
+        let mut head_commit_author = None;
+
+        // If repo root found, try to get repo details from git.
+        if let Some(git_repo) = bundle_repo_options
+            .repo_root
+            .as_ref()
+            .and_then(|dir| gix::open(dir).ok())
+        {
+            bundle_repo_options.repo_url = bundle_repo_options.repo_url.or_else(|| {
+                git_repo
+                    .config_snapshot()
+                    .string_by_key(GIT_REMOTE_ORIGIN_URL_CONFIG)
+                    .map(|s| s.to_string())
+            });
+
+            if let Ok(mut git_head) = git_repo.head() {
+                bundle_repo_options.repo_head_branch = bundle_repo_options
+                    .repo_head_branch
+                    .or_else(|| git_head.referent_name().map(|s| s.as_bstr().to_string()))
+                    .or_else(|| {
+                        Self::git_head_branch_from_remote_branches(&git_repo)
+                            .ok()
+                            .flatten()
+                    });
+
+                bundle_repo_options.repo_head_sha = bundle_repo_options
+                    .repo_head_sha
+                    .or_else(|| git_head.id().map(|id| id.to_string()));
+
+                if let Ok(commit) = git_head.peel_to_commit_in_place() {
+                    bundle_repo_options.repo_head_commit_epoch = bundle_repo_options
+                        .repo_head_commit_epoch
+                        .or_else(|| commit.time().ok().map(|time| time.seconds));
+                    head_commit_message = commit.message().map(|msg| msg.title.to_string()).ok();
+                    head_commit_author = commit.author().ok().map(|signature| signature.to_owned());
+                }
+            }
+        }
+
+        // Require URL which should be known at this point.
+        let repo_url = bundle_repo_options
+            .repo_url
+            .context("failed to get repo URL")?;
+        let repo_url_parts =
+            RepoUrlParts::from_url(&repo_url).context("failed to parse repo URL")?;
+        let (repo_head_author_name, repo_head_author_email) = head_commit_author
+            .as_ref()
+            .map(|a| (a.name.to_string(), a.email.to_string()))
+            .unwrap_or_default();
+        Ok(BundleRepo {
+            repo: repo_url_parts,
+            repo_root: bundle_repo_options
+                .repo_root
+                .and_then(|p| p.to_str().map(String::from))
+                .unwrap_or_default(),
+            repo_url,
+            repo_head_branch: bundle_repo_options.repo_head_branch.unwrap_or_default(),
+            repo_head_sha: bundle_repo_options.repo_head_sha.unwrap_or_default(),
+            repo_head_commit_epoch: bundle_repo_options
+                .repo_head_commit_epoch
+                .unwrap_or_default(),
+            repo_head_commit_message: head_commit_message.unwrap_or_default(),
+            repo_head_author_name,
+            repo_head_author_email,
+        })
+    }
+
+    fn git_head_branch_from_remote_branches(
+        git_repo: &Repository,
+    ) -> anyhow::Result<Option<String>> {
+        for remote_branch in git_repo
+            .references()?
+            .remote_branches()?
+            .filter_map(Result::ok)
+        {
+            if let Some(target_id) = remote_branch.target().try_id() {
+                if target_id.as_bytes() == remote_branch.id().as_bytes() {
+                    return Ok(remote_branch.name().to_path().to_str().map(String::from));
+                }
+            }
+        }
+        Ok(None)
+    }
+}


I re-wrote this to be clearer, have less variables getting defined and mutated, and easier to make bindings for other languages

dfrankland · 2024-10-02T00:22:22Z

context/src/repo/mod.rs

+pub struct RepoUrlParts {
+    pub host: String,
+    pub owner: String,
+    pub name: String,
+}
+
+impl RepoUrlParts {


I renamed this from Repo since it was confusing seeing repo.repo everywhere. This is really only the results of parsing a repo URL.

See my nit above, but this does conflict with how we've named this elsewhere.

dfrankland · 2024-10-02T00:24:47Z

context/src/repo/mod.rs

+        let re1 = Regex::new(r"^(ssh|git|http|https|ftp|ftps)://([^/]*?@)?([^/]*)/(.+)/([^/]+)")?;
+        let re2 = Regex::new(r"^([^/]*?@)([^/]*):(.+)/([^/]+)")?;
+
+        let parts = if re1.is_match(url) {
+            let caps = re1.captures(url).expect("failed to parse url");
+            if caps.len() != 6 {
+                return Err(anyhow::anyhow!(
+                    "Invalid repo url format. Expected 6 parts: {:?} (url: {})",
+                    caps,
+                    url
+                ));
+            }
+            let domain = caps.get(3).map(|m| m.as_str()).unwrap_or("");
+            let owner = caps.get(4).map(|m| m.as_str()).unwrap_or("");
+            let name = caps.get(5).map(|m| m.as_str()).unwrap_or("");
+            (domain, owner, name)
+        } else if re2.is_match(url) {
+            let caps = re2.captures(url).expect("failed to parse url");
+            if caps.len() != 5 {
+                return Err(anyhow::anyhow!(
+                    "Invalid repo url format. Expected 6 parts: {:?} (url: {})",
+                    caps,
+                    url
+                ));
+            }
+            let domain = caps.get(2).map(|m| m.as_str()).unwrap_or("");
+            let owner = caps.get(3).map(|m| m.as_str()).unwrap_or("");
+            let name = caps.get(4).map(|m| m.as_str()).unwrap_or("");
+            (domain, owner, name)
+        } else {
+            return Err(anyhow::anyhow!("Invalid repo url format: {}", url));
+        };
+
+        let host = parts.0.trim().to_string();
+        let owner = parts.1.trim().to_string();
+        let name = parts
+            .2
+            .trim()
+            .trim_end_matches('/')
+            .trim_end_matches(".git")
+            .to_string();
+
+        if host.is_empty() || owner.is_empty() || name.is_empty() {
+            return Err(anyhow::anyhow!(
+                "Invalid repo url format. Expected non-empty parts: {:?} (url: {})",
+                parts,
+                url
+            ));
+        }
+
+        Ok(Self { host, owner, name })


I only copied this logic, but we should probably follow up with some best practices here, like:

Use the url crate

Use lazy_static for regexes

dfrankland · 2024-10-02T00:25:24Z

context/tests/repo.rs

Pretty much completely copied without logic changes—just updated to use RepoUrlParts

dfrankland · 2024-10-02T00:26:09Z

test_utils/Cargo.toml

Finally split this out from cli to make it easier to reuse some utils in tests for the context crate

dfrankland · 2024-10-02T00:26:36Z

api/Cargo.toml

This crate got split out from cli to be reused in test_utils

gnalh

Overall LGTM, just a few nits on names.

gnalh · 2024-10-02T18:41:54Z

cli/src/clients.rs

 };
+use context::repo::RepoUrlParts;


Nit: This no longer aligns with how we name it in our other repos. We should be consistent.

gnalh · 2024-10-02T18:42:26Z

cli/src/clients.rs

@@ -16,7 +16,7 @@ pub async fn create_trunk_repo(
    origin: &str,
    api_token: &str,
    org_slug: &str,
-    repo: &Repo,
+    repo: &RepoUrlParts,


Nit, update the parameter to match if we stick with this new name type.

Suggested change

repo: &RepoUrlParts,

repo_url_parts: &RepoUrlParts,

gnalh · 2024-10-02T18:45:05Z

context/src/repo/mod.rs

+pub struct RepoUrlParts {
+    pub host: String,
+    pub owner: String,
+    pub name: String,
+}
+
+impl RepoUrlParts {


See my nit above, but this does conflict with how we've named this elsewhere.

dfrankland · 2024-10-03T15:18:43Z

I'm planning to address nits by aliasing RepoUrlParts to Repo within the cli crate in a follow up PR

dfrankland changed the title ~~split out api, test_utils, and bundlerepo from cli~~ 3. split out api, test_utils, and bundlerepo from cli Oct 1, 2024

dfrankland mentioned this pull request Oct 1, 2024

4. add bundle repo validator #110

Merged

dfrankland commented Oct 2, 2024

View reviewed changes

dfrankland marked this pull request as ready for review October 2, 2024 00:30

dfrankland requested review from riya-n and gnalh October 2, 2024 00:30

dfrankland force-pushed the dylan/add-context-crate branch from c48be49 to 9d7fa72 Compare October 2, 2024 16:19

dfrankland force-pushed the dylan/split-api-test-utils-and-bundlrepo-from-cli branch from 7924e52 to c72e8f9 Compare October 2, 2024 16:22

Base automatically changed from dylan/add-context-crate to main October 2, 2024 16:50

split out api, test_utils, and bundlerepo from cli

a1702f1

dfrankland force-pushed the dylan/split-api-test-utils-and-bundlrepo-from-cli branch from c72e8f9 to a1702f1 Compare October 2, 2024 16:51

gnalh approved these changes Oct 2, 2024

View reviewed changes

riya-n approved these changes Oct 2, 2024

View reviewed changes

dfrankland mentioned this pull request Oct 3, 2024

Revert "Update BundleUpload status for observability" #112

Merged

dfrankland merged commit 7ca7dd7 into main Oct 3, 2024
6 checks passed

dfrankland deleted the dylan/split-api-test-utils-and-bundlrepo-from-cli branch October 3, 2024 15:18

dfrankland mentioned this pull request Oct 3, 2024

6. document and alias RepoUrlParts to Repo #113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3. split out api, test_utils, and bundlerepo from cli #109

3. split out api, test_utils, and bundlerepo from cli #109

dfrankland commented Oct 1, 2024 •

edited

Loading

trunk-staging-io bot commented Oct 1, 2024 •

edited

Loading

dfrankland Oct 2, 2024

dfrankland Oct 2, 2024 •

edited

Loading

dfrankland Oct 2, 2024

gnalh Oct 2, 2024

dfrankland Oct 2, 2024 •

edited

Loading

dfrankland Oct 2, 2024 •

edited

Loading

dfrankland Oct 2, 2024 •

edited

Loading

dfrankland Oct 2, 2024

gnalh left a comment

gnalh Oct 2, 2024

gnalh Oct 2, 2024

gnalh Oct 2, 2024

dfrankland commented Oct 3, 2024

3. split out api, test_utils, and bundlerepo from cli #109

3. split out api, test_utils, and bundlerepo from cli #109

Conversation

dfrankland commented Oct 1, 2024 • edited Loading

trunk-staging-io bot commented Oct 1, 2024 • edited Loading

dfrankland Oct 2, 2024

Choose a reason for hiding this comment

dfrankland Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

dfrankland Oct 2, 2024

Choose a reason for hiding this comment

gnalh Oct 2, 2024

Choose a reason for hiding this comment

dfrankland Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

dfrankland Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

dfrankland Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

dfrankland Oct 2, 2024

Choose a reason for hiding this comment

gnalh left a comment

Choose a reason for hiding this comment

gnalh Oct 2, 2024

Choose a reason for hiding this comment

gnalh Oct 2, 2024

Choose a reason for hiding this comment

gnalh Oct 2, 2024

Choose a reason for hiding this comment

dfrankland commented Oct 3, 2024

dfrankland commented Oct 1, 2024 •

edited

Loading

trunk-staging-io bot commented Oct 1, 2024 •

edited

Loading

dfrankland Oct 2, 2024 •

edited

Loading

dfrankland Oct 2, 2024 •

edited

Loading

dfrankland Oct 2, 2024 •

edited

Loading

dfrankland Oct 2, 2024 •

edited

Loading