-
Notifications
You must be signed in to change notification settings - Fork 270
fix: support lowercase proxy environment variables fallback #559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: support lowercase proxy environment variables fallback #559
Conversation
BUGFIX: fix AutoGetWithGetter will never hit cache issue.
feat: Agent plugin add features tag
support to use mysql as plugin db
…ible make pathStyle param configurable when using s3 compatible storage
feat: simplify the process in db migrate & fix comment error
chore: upgrade ants to v2
…internal-use Add get/delete implement, for internal use.
feat: add ConvertAnyMap function and corresponding tests for map conv…
…hon_version fix: some time uv venv wrong python version
fix: thread safety issue and len inaccuracy in Map
* fix: align multimodal permission mapping * fix: implement mock interface for multimodal embeddings * fix: support multimodal embedding * fix: incorrect reference
…and upgrade (langgenius#526) * add locking to prevent simultaneous installations of the same plugin * ensure proper unlocking of keys in case of errors during installation and upgrade * handle database not found error in DeletePluginInstallationItemFromTask
* fix: remove tenant ID reference from plugin readme service * fix: rename PluginReadme to PluginReadmeRecord to remove tenant ID reference * fix: handle error when saving plugin readme map to database * fix: remove TenantId from FetchPluginReadme request binding * fix: standardize error message casing for plugin unique identifier validation
…anggenius#536) * refactor: remove uuid-ossp dependency and update ID generation logic * fix: condition error
* fix: plugin log not display after refactor * Update internal/core/local_runtime/notifier_logger.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * feat: add logging support for local runtime instance events --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Yeuoly <[email protected]>
…anggenius#539) * Update internal/types/app/config.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update internal/server/server.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* feat: add support for PgBouncer in database initialization * refactor: consolidate gorm configuration for database connection * Add MySQL and multi-driver DB integration tests (langgenius#535) * feat: streamline integration tests by using a centralized docker-compose file * fix: add timeout * refactor: replace inline service definitions with docker-compose action for integration tests * fix: update pgbouncer image and environment variable names for consistency
…fy instance state handling (langgenius#544)
…e not found (langgenius#549) * fix PluginDaemonInternalServerError: no available node, plugin runtime not found Fix the bug where after a plugin is successfully upgraded, new requests result in the plugin daemon service responding with "no available node, plugin runtime not found". The root cause is that when the plugin is upgraded successfully, the corresponding value in Redis is not updated. Consequently, when a new request arrives, it reads the outdated plugin version from Redis and fails to locate the correct runtime. * raise error if failed
…s#552) * use slog instead of log package and format to new log schema * update the environment name to LOG_OUTPUT_FORMAT * add the env to .env.example * fix log reference error * change the order of milldlewares * delete unused code * fix the concurrently session potential race condition * fix the log format in tests * update the duplicate code * refactor: convert log functions to slog structured format - Change log.Error/Info/Warn/Debug/Panic to accept msg + key-value pairs - Remove printf-style formatting from log functions - Update log calls in internal/cluster, internal/db, internal/core/session_manager - Remove unused 'initialized' variable from log package - Remaining files will be updated in follow-up commits 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * refactor: update all log call sites to use slog structured format Convert all log.Error, log.Info, log.Warn, log.Debug, and log.Panic calls from printf-style formatting to slog key-value pairs. Before: log.Error("failed to do something: %s", err.Error()) After: log.Error("failed to do something", "error", err) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * refactor: update cmd/ log calls to use slog structured format 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * feat: implement GnetLogger for structured logging in gnet * refactor: remove deprecated log visibility functions and related calls * feat: enhance session management with trace and identity context propagation * feat: implement serverless transaction handler and writer for plugin runtime * refactor: rename context field to traceCtx in RealBackwardsInvocation --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: Yeuoly <[email protected]>
- Add fallback to lowercase http_proxy/https_proxy/no_proxy when uppercase versions are empty - Fixes langgenius/dify#18752
Summary of ChangesHello @fghpdf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's proxy configuration handling by introducing a fallback mechanism for environment variables. It ensures that if standard uppercase proxy variables like HTTP_PROXY are not defined, the system will automatically check for and use their lowercase equivalents such as http_proxy. This change improves the robustness and flexibility of proxy settings, accommodating various system configurations without breaking existing behavior. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds fallback support for lowercase proxy environment variables (http_proxy, https_proxy, no_proxy) when their uppercase counterparts are not set. The implementation is sound and achieves the goal described. I've included a couple of suggestions to refactor the changes by reusing an existing helper function. This would improve code maintainability and reduce duplication. Overall, this is a good and useful addition.
internal/types/app/default.go
Outdated
| setDefaultStringFromEnv(&config.HttpProxy, "http_proxy") | ||
| setDefaultStringFromEnv(&config.HttpsProxy, "https_proxy") | ||
| setDefaultStringFromEnv(&config.NoProxy, "no_proxy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve code reuse and maintainability, you can leverage the existing setDefaultString function here. This avoids introducing a new, specialized helper function (setDefaultStringFromEnv). The os.Getenv call is inexpensive, and this approach keeps the helper functions more generic.
I've added another comment suggesting the removal of the setDefaultStringFromEnv function definition, which would be redundant with this change.
| setDefaultStringFromEnv(&config.HttpProxy, "http_proxy") | |
| setDefaultStringFromEnv(&config.HttpsProxy, "https_proxy") | |
| setDefaultStringFromEnv(&config.NoProxy, "no_proxy") | |
| setDefaultString(&config.HttpProxy, os.Getenv("http_proxy")) | |
| setDefaultString(&config.HttpsProxy, os.Getenv("https_proxy")) | |
| setDefaultString(&config.NoProxy, os.Getenv("no_proxy")) |
internal/types/app/default.go
Outdated
| func setDefaultStringFromEnv(value *string, envKey string) { | ||
| if *value == "" { | ||
| *value = os.Getenv(envKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9c4cf3e to
15503bc
Compare
Description
This PR adds support for lowercase proxy environment variables as a fallback when uppercase versions are not set.
Changes
SetDefault()method to check lowercasehttp_proxy,https_proxy, andno_proxywhen uppercase versions (HTTP_PROXY,HTTPS_PROXY,NO_PROXY) are emptysetDefaultStringFromEnv()to read from environment variablesFixes
Fixes langgenius/dify#18752
Testing
Behavior
HTTP_PROXYis set, it will be used (backward compatible)HTTP_PROXYis empty buthttp_proxyis set,http_proxywill be usedHTTPS_PROXY/https_proxyandNO_PROXY/no_proxy