feat(config): support GitHub proxy via MAA_GITHUB_PROXY env var#541
Open
lingdiansr wants to merge 3 commits into
Open
feat(config): support GitHub proxy via MAA_GITHUB_PROXY env var#541lingdiansr wants to merge 3 commits into
lingdiansr wants to merge 3 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些整体性的反馈：
github_proxy()访问器可以对返回值进行规范化处理（例如使用trim_end_matches('/')），这样像apply_github_proxy这样的调用方就不需要每次都记得自己去移除结尾的斜杠。github_proxy的测试用例把std::env::set_var/remove_var包裹在unsafe块中，这是不必要的，而且可能会让人困惑；你可以去掉unsafe，如果需要的话，把测试标记为串行执行以避免测试之间的相互干扰。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `github_proxy()` accessor could normalize the value (e.g., `trim_end_matches('/')`) so callers like `apply_github_proxy` don't all have to remember to strip trailing slashes themselves.
- The `github_proxy` tests wrap `std::env::set_var`/`remove_var` in `unsafe` blocks, which is unnecessary and may be confusing; you can drop the `unsafe` and, if needed, mark the test as serial to avoid cross-test interference.
## Individual Comments
### Comment 1
<location path="crates/maa-cli/src/installer/maa_core.rs" line_range="34-40" />
<code_context>
+ url.split('/').nth(2) == Some("github.com") && url.contains("/releases/download/")
+}
+
+fn apply_github_proxy(manifest: &mut VersionManifest<core::Details>, proxy: &str) {
+ let proxy = proxy.trim_end_matches('/');
+ for asset in &mut manifest.details.assets {
+ if is_github_release_url(&asset.browser_download_url) {
+ asset.browser_download_url = format!("{proxy}/{}", asset.browser_download_url);
+ }
+ for mirror in &mut asset.mirrors {
+ if is_github_release_url(mirror) {
+ *mirror = format!("{proxy}/{mirror}");
</code_context>
<issue_to_address>
**issue:** 需要防范空 proxy 值，以避免生成无效的 URL。
当 `github_proxy` 被配置为空字符串时，`CLI_CONFIG.github_proxy()` 会返回 `Some("")`，并且在调用 `trim_end_matches('/')` 之后，该值仍然是空的。此时 `apply_github_proxy` 会生成类似 `/https://github.com/...` 的 URL。请在调用该函数前将空的 proxy 视为 `None`，或者在这里当 `proxy` 为空时提前返回，以避免生成无效的 URL。
</issue_to_address>
### Comment 2
<location path="crates/maa-cli/src/config/cli/mod.rs" line_range="47-51" />
<code_context>
+ ///
+ /// Returns the value of the `MAA_GITHUB_PROXY` environment variable if set
+ /// and non-empty, otherwise falls back to the configured value.
+ pub fn github_proxy(&self) -> Option<String> {
+ std::env::var("MAA_GITHUB_PROXY")
+ .ok()
+ .filter(|s| !s.is_empty())
+ .or_else(|| self.github_proxy.clone())
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 规范空配置值，并考虑避免在 `github_proxy` 中进行重复分配。
当前实现会把配置中的 `github_proxy = ""` 当作一个真实的代理（`Some("")`），而环境变量则会忽略空字符串。建议对配置进行规范化，将空字符串视为 `None`，以保持行为一致。另外，这个实现每次调用都会分配一个新的 `String`；如果这是一个热路径，返回 `Option<&str>` 或 `Option<Cow<'_, str>>` 可以避免重复克隆。
建议实现：
```rust
/// Get the GitHub proxy prefix.
///
/// Returns the value of the `MAA_GITHUB_PROXY` environment variable if set
/// and non-empty, otherwise falls back to the configured value.
///
/// Empty strings from either the environment or the configuration are
/// treated as if no proxy were configured.
pub fn github_proxy(&self) -> Option<std::borrow::Cow<'_, str>> {
// Prefer a non-empty value from the environment (owned string)
if let Ok(env_val) = std::env::var("MAA_GITHUB_PROXY") {
if !env_val.is_empty() {
return Some(std::borrow::Cow::Owned(env_val));
}
}
// Fall back to a non-empty configured value, borrowed to avoid cloning
self.github_proxy
.as_deref()
.filter(|s| !s.is_empty())
.map(std::borrow::Cow::Borrowed)
```
1. 如果你更喜欢更短的类型名，可以在模块顶部添加 `use std::borrow::Cow;`，然后把函数签名和函数体中的 `std::borrow::Cow<'_, str>` 替换为 `Cow<'_, str>`。
2. 这个改动会把 `github_proxy` 的返回类型从 `Option<String>` 改为 `Option<Cow<'_, str>>`。所有目前期望 `Option<String>` 的调用点都需要更新：
- 如果它们只需要借用的字符串，使用 `if let Some(proxy) = cfg.github_proxy() { /* &proxy */ }`。
- 如果需要拥有所有权的 `String`，调用 `cfg.github_proxy().map(|c| c.into_owned())`。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据这些反馈改进后续的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The
github_proxy()accessor could normalize the value (e.g.,trim_end_matches('/')) so callers likeapply_github_proxydon't all have to remember to strip trailing slashes themselves. - The
github_proxytests wrapstd::env::set_var/remove_varinunsafeblocks, which is unnecessary and may be confusing; you can drop theunsafeand, if needed, mark the test as serial to avoid cross-test interference.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `github_proxy()` accessor could normalize the value (e.g., `trim_end_matches('/')`) so callers like `apply_github_proxy` don't all have to remember to strip trailing slashes themselves.
- The `github_proxy` tests wrap `std::env::set_var`/`remove_var` in `unsafe` blocks, which is unnecessary and may be confusing; you can drop the `unsafe` and, if needed, mark the test as serial to avoid cross-test interference.
## Individual Comments
### Comment 1
<location path="crates/maa-cli/src/installer/maa_core.rs" line_range="34-40" />
<code_context>
+ url.split('/').nth(2) == Some("github.com") && url.contains("/releases/download/")
+}
+
+fn apply_github_proxy(manifest: &mut VersionManifest<core::Details>, proxy: &str) {
+ let proxy = proxy.trim_end_matches('/');
+ for asset in &mut manifest.details.assets {
+ if is_github_release_url(&asset.browser_download_url) {
+ asset.browser_download_url = format!("{proxy}/{}", asset.browser_download_url);
+ }
+ for mirror in &mut asset.mirrors {
+ if is_github_release_url(mirror) {
+ *mirror = format!("{proxy}/{mirror}");
</code_context>
<issue_to_address>
**issue:** Guard against empty proxy values to avoid generating invalid URLs.
When `github_proxy` is configured as an empty string, `CLI_CONFIG.github_proxy()` returns `Some("")`, and after `trim_end_matches('/')` the value is still empty. `apply_github_proxy` will then produce URLs like `/https://github.com/...`. Please either treat an empty proxy as `None` before calling this function or early-return here when `proxy` is empty to avoid generating invalid URLs.
</issue_to_address>
### Comment 2
<location path="crates/maa-cli/src/config/cli/mod.rs" line_range="47-51" />
<code_context>
+ ///
+ /// Returns the value of the `MAA_GITHUB_PROXY` environment variable if set
+ /// and non-empty, otherwise falls back to the configured value.
+ pub fn github_proxy(&self) -> Option<String> {
+ std::env::var("MAA_GITHUB_PROXY")
+ .ok()
+ .filter(|s| !s.is_empty())
+ .or_else(|| self.github_proxy.clone())
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Normalize empty config values and consider avoiding repeated allocations in `github_proxy`.
This currently treats `github_proxy = ""` in the config as a real proxy (`Some("")`), unlike the env var, which ignores empty strings. Consider normalizing config so empty strings are treated as `None` for consistency. Also, this allocates a new `String` on every call; if it’s on a hot path, returning `Option<&str>` or `Option<Cow<'_, str>>` would avoid repeated cloning.
Suggested implementation:
```rust
/// Get the GitHub proxy prefix.
///
/// Returns the value of the `MAA_GITHUB_PROXY` environment variable if set
/// and non-empty, otherwise falls back to the configured value.
///
/// Empty strings from either the environment or the configuration are
/// treated as if no proxy were configured.
pub fn github_proxy(&self) -> Option<std::borrow::Cow<'_, str>> {
// Prefer a non-empty value from the environment (owned string)
if let Ok(env_val) = std::env::var("MAA_GITHUB_PROXY") {
if !env_val.is_empty() {
return Some(std::borrow::Cow::Owned(env_val));
}
}
// Fall back to a non-empty configured value, borrowed to avoid cloning
self.github_proxy
.as_deref()
.filter(|s| !s.is_empty())
.map(std::borrow::Cow::Borrowed)
```
1. If you prefer shorter type names, you can add `use std::borrow::Cow;` near the top of the module and then change the function signature and body to use `Cow<'_, str>` instead of `std::borrow::Cow<'_, str>`.
2. This change alters the return type of `github_proxy` from `Option<String>` to `Option<Cow<'_, str>>`. All call sites that currently expect an `Option<String>` will need to be updated:
- If they only need a borrowed string, use `if let Some(proxy) = cfg.github_proxy() { /* &proxy */ }`.
- If they require an owned `String`, call `cfg.github_proxy().map(|c| c.into_owned())`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
==========================================
+ Coverage 71.86% 71.95% +0.08%
==========================================
Files 69 69
Lines 6412 6443 +31
Branches 6412 6443 +31
==========================================
+ Hits 4608 4636 +28
- Misses 1477 1478 +1
- Partials 327 329 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Normalize empty config values to None in github_proxy() - Trim trailing slashes in github_proxy() so callers don't need to - Guard against empty proxy in apply_github_proxy to avoid invalid URLs - Add empty_proxy_is_noop test - Update test expectations for normalized (no-trailing-slash) values
Member
|
我个人不是很喜欢使用使用环境变量来做配置，因为实现和测试起来比较麻烦。我想知道其他 CLI 使用类似的环境变量来做 GITHUB 的代理吗? 此外如果只是对于 Core releases 来做的话，是不是放在 Core 这个 section 里面更合适? |
Contributor
Author
其他cli确实每怎么见过专给github设置代理的变量的，因为都是外国开发者没怎么考虑过，都使用通用的http_proxy。 #[test]
fn env_via_subprocess() {
let exe = std::env::current_exe().unwrap();
let output = std::process::Command::new(exe)
.env("MAA_GITHUB_PROXY", "https://env/")
.arg("--test-threads=1")
.arg("subprocess_github_proxy")
.output()
.unwrap();
assert!(output.status.success());
}
#[test]
fn subprocess_github_proxy() {
// 这个测试只在子进程中运行
let config = CLIConfig::default();
assert_eq!(config.github_proxy(), Some("https://env".into()));
}我不是很喜欢一直开启代理软件。 |
Member
|
我觉得可以提供这个功能，不过我倾向于要么用环境变量，要么用设置二选一，设置测试起来比较方便。此外我觉得，其实这个可以用 ureq 的 milldeware 来做，可能这样比较方便, 而不是在设置里面到处打补丁。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
功能说明
支持通过环境变量
MAA_GITHUB_PROXY或配置文件cli.toml中的github_proxy字段设置 GitHub 代理前缀，以加速maa install和maa update时的 maa-core releases 下载。使用方式
环境变量（优先级最高）
export MAA_GITHUB_PROXY=https://hk.gh-proxy.org/ maa update配置文件
实现细节
CoreManifest::from_reader中解析 version manifest 后，对所有 host 为github.com且路径包含/releases/download/的 URL 添加代理前缀browser_download_url和 mirrors 中的 GitHub releases 链接download_url配置可手动指定代理，不额外修改修改文件
crates/maa-cli/src/config/cli/mod.rs：新增github_proxy顶层配置项；github_proxy()方法读取MAA_GITHUB_PROXY环境变量（优先）或配置值crates/maa-cli/src/installer/maa_core.rs：新增is_github_release_url()和apply_github_proxy()；在 manifest 解析后调用crates/maa-cli/config_examples/cli.toml：添加配置示例与注释测试
cargo +nightly fmt：通过cargo clippy：零警告cargo test -p maa-cli：312 passed, 0 failedSummary by Sourcery
为 maa-cli 添加可配置的 GitHub 代理支持，用于下载 maa-core 的发布版本，从而加速安装和更新操作。
New Features:
MAA_GITHUB_PROXY环境变量或cli.toml中的github_proxy字段，允许配置 GitHub Releases 下载代理。Enhancements:
cli.toml配置文件中记录github_proxy选项。Tests:
github_proxy配置中环境变量优先于配置文件的优先级规则。Original summary in English
Summary by Sourcery
Add configurable GitHub proxy support for maa-core release downloads in maa-cli to speed up install and update operations.
New Features:
Enhancements:
Tests: