but-forge: Improve forge determination#12564
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
d343be2 to
abb2b46
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves forge type determination in but-forge by adding a fallback mechanism that matches repository hosts against custom hosts from configured forge accounts. Previously, the system only used naive string matching against well-known forge domains (github.com, gitlab.com, etc.). Now, if that fails, it checks if any configured GitHub Enterprise or GitLab Self-Hosted account has a custom host that matches the repository URL.
Changes:
- Added
custom_host()methods to GitHub and GitLab account identifiers to retrieve custom forge hosts - Implemented host normalization and matching logic to compare repository hosts with account custom hosts, including subdomain matching
- Added comprehensive test coverage for the new matching behavior
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but-github/src/token.rs | Added custom_host() method to retrieve the host for GitHub Enterprise accounts |
| crates/but-gitlab/src/token.rs | Added custom_host() method to retrieve the host for GitLab Self-Hosted accounts |
| crates/but-forge/src/lib.rs | Implemented forge determination fallback with host matching logic, normalization, and account loading; added comprehensive tests |
| crates/but-forge/Cargo.toml | Added but-path dependency for accessing app data directory |
| Cargo.lock | Updated to include the new but-path dependency |
|
@estib-vega I've opened a new pull request, #12567, to work on those changes. Once the pull request is ready, I'll request review from you. |
abb2b46 to
8152867
Compare
8152867 to
9c1df49
Compare
| }; | ||
|
|
||
| let forge_repo_info = but_forge::derive_forge_repo_info(&remote_url); | ||
| let forge_accounts = but_forge::get_all_forge_accounts()?; |
There was a problem hiding this comment.
The new implementation introduces error-prone behavior. If get_all_forge_accounts() fails (due to storage issues, permissions, or other errors), the entire target_to_base_branch function will fail, which breaks get_base_branch_data and all its callers (20+ locations). Previously, derive_forge_repo_info would simply return None if forge detection failed, allowing operations to continue. Consider wrapping the call in .unwrap_or_default() or logging and continuing with an empty accounts list to maintain backward compatibility.
| let forge_accounts = but_forge::get_all_forge_accounts()?; | |
| let forge_accounts = but_forge::get_all_forge_accounts().unwrap_or_default(); |
| pub fn get_all_forge_accounts() -> anyhow::Result<Vec<ForgeUser>> { | ||
| if let Ok(handle) = tokio::runtime::Handle::try_current() { | ||
| tokio::task::block_in_place(|| handle.block_on(get_all_forge_accounts_async())) | ||
| } else { | ||
| let runtime = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; | ||
| runtime.block_on(get_all_forge_accounts_async()) | ||
| } | ||
| } |
There was a problem hiding this comment.
This function is called on every invocation of target_to_base_branch, which is used in multiple hot paths including get_base_branch_data (20+ call sites). Each call creates/blocks on a tokio runtime and performs storage I/O to load accounts. Consider caching the accounts list or passing it as a parameter from callers that already have it, to avoid repeated storage access overhead.
There was a problem hiding this comment.
This is actually true.
A better fix would be to completely separate the base branch data from the forge information.
I'll create a third function that simply takes the base branch data and returns the forge information.
This will require changes in the FE.
| for gh_account in gh_accounts { | ||
| forge_users.push(ForgeUser::GitHub(gh_account)); | ||
| } | ||
|
|
||
| for gl_account in gl_accounts { | ||
| forge_users.push(ForgeUser::GitLab(gl_account)); | ||
| } | ||
|
|
There was a problem hiding this comment.
The vector construction can be simplified using extend. Replace the explicit loops with: forge_users.extend(gh_accounts.into_iter().map(ForgeUser::GitHub)); and forge_users.extend(gl_accounts.into_iter().map(ForgeUser::GitLab)); for more idiomatic Rust.
| for gh_account in gh_accounts { | |
| forge_users.push(ForgeUser::GitHub(gh_account)); | |
| } | |
| for gl_account in gl_accounts { | |
| forge_users.push(ForgeUser::GitLab(gl_account)); | |
| } | |
| forge_users.extend(gh_accounts.into_iter().map(ForgeUser::GitHub)); | |
| forge_users.extend(gl_accounts.into_iter().map(ForgeUser::GitLab)); |
9c1df49 to
6beb6b0
Compare
Old GitHub secrets can now stop being migrated.
Try to determine the forge type for a given URL, by matching it naively (as we were always doing) and fallback to matching it to the custom hosts for the self-hosted or enterprise accounts.
6beb6b0 to
2dd3a38
Compare
Try to determine the forge type for a given URL, by matching it naively
(as we were always doing) and fallback to matching it to the custom
hosts for the self-hosted or enterprise accounts.