Skip to content

Conversation

@pdecat
Copy link
Contributor

@pdecat pdecat commented Nov 14, 2025

User description

The changes in PR #2087 which are included in v0.31 prevent Dynaconf fresh vars from working properly.

For context, here's my setup with the codiumai/pr-agent:0.31-gitlab_webhook image:

In kubernetes pod, this environment variable is set:

FRESH_VARS_FOR_DYNACONF='["GITLAB"]'

Note: that this has to be uppercase as mentioned here.

In /app/pr_agent/settings_prod/.secrets.toml which is mounted from a kubernetes secret:

[gitlab]
personal_access_token = "xxx"
shared_secret = "xxx"

With v0.30, every time the secret is updated (every day in our case as we use short lived Gitlab tokens), the file is refreshed, and Dynaconf picks the changes perfectly without restarting neither the pod nor the pr-agent process.

The first commit in this PR only introduces unit tests to demonstrate the issue in Github workflow execution.

The fix is in second commit pushed after the first workflow execution: 312134a


PR Type

Tests, Bug fix


Description

  • Add comprehensive unit tests for Dynaconf fresh_vars functionality

  • Tests verify fresh_vars works with custom_merge_loader configuration

  • Detect regression where fresh_vars stopped working after PR Set a custom toml loader for Dynaconf #2087

  • Cover GitLab credentials reload scenario with file modifications


Diagram Walkthrough

flowchart LR
  A["Test Suite"] --> B["Fresh Vars Functionality"]
  B --> C["GitLab Scenario Tests"]
  B --> D["Custom Loader Integration"]
  B --> E["Basic Functionality Tests"]
  C --> F["Token Reload Detection"]
  D --> G["Core Loaders Disabled"]
  E --> H["Environment Variable Config"]
Loading

File Walkthrough

Relevant files
Tests
test_fresh_vars_functionality.py
Comprehensive fresh_vars functionality test suite               

tests/unittest/test_fresh_vars_functionality.py

  • Add 520 lines of comprehensive unit tests for Dynaconf fresh_vars
    feature
  • Create three test classes covering GitLab credentials scenario, custom
    loader integration, and basic functionality
  • Include critical test test_fresh_vars_without_core_loaders to detect
    if fresh_vars is broken with custom_merge_loader
  • Test file modification detection and value reloading with multiple
    scenarios and edge cases
+520/-0 

@qodo-merge-for-open-source
Copy link
Contributor

qodo-merge-for-open-source bot commented Nov 14, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Consistent Naming Conventions

Objective: All new variables, functions, and classes must follow the project's established naming
standards

Status: Passed

Single Responsibility for Functions

Objective: Each function should have a single, well-defined responsibility

Status: Passed

When relevant, utilize early return

Objective: In a code snippet containing multiple logic conditions (such as 'if-else'), prefer an
early return on edge cases than deep nesting

Status: Passed

🔴
No Dead or Commented-Out Code

Objective: Keep the codebase clean by ensuring all submitted code is active and necessary

Status:
Commented-out code present: Line 42 contains commented-out code os.utime(file_path, (new_time, new_time)) that should
be removed or uncommented if needed.

Referred Code
# os.utime(file_path, (new_time, new_time))
Robust Error Handling

Objective: Ensure potential errors and edge cases are anticipated and handled gracefully throughout
the code

Status:
Missing error handling: File operations like write_text() and rmtree() lack error handling for potential I/O
failures, though this may be acceptable in test code.

Referred Code
self.secrets_file.write_text(content)
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@pdecat
Copy link
Contributor Author

pdecat commented Nov 14, 2025

First test workflow execution:

=========================== short test summary info ============================
FAILED tests/unittest/test_fresh_vars_functionality.py::TestFreshVarsGitLabScenario::test_gitlab_personal_access_token_reload
FAILED tests/unittest/test_fresh_vars_functionality.py::TestFreshVarsGitLabScenario::test_gitlab_shared_secret_reload
FAILED tests/unittest/test_fresh_vars_functionality.py::TestFreshVarsGitLabScenario::test_gitlab_multiple_fields_reload
FAILED tests/unittest/test_fresh_vars_functionality.py::TestFreshVarsCustomLoaderIntegration::test_fresh_vars_without_core_loaders
FAILED tests/unittest/test_fresh_vars_functionality.py::TestFreshVarsCustomLoaderIntegration::test_custom_loader_respects_fresh_vars
FAILED tests/unittest/test_fresh_vars_functionality.py::TestFreshVarsBasicFunctionality::test_fresh_vars_from_environment_variable
FAILED tests/unittest/test_fresh_vars_functionality.py::TestFreshVarsBasicFunctionality::test_gitlab_credentials_not_cached_when_fresh
============ 7 failed, 192 passed, 6 skipped, 24 warnings in 5.71s =============

https://github.com/qodo-ai/pr-agent/actions/runs/19370299280/job/55424179340?pr=2104#step:5:579

@qodo-merge-for-open-source
Copy link
Contributor

qodo-merge-for-open-source bot commented Nov 14, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Use correct helper method for tests

In TestFreshVarsGitLabScenario, replace calls to the global
create_dynaconf_with_custom_loader with the class method
self.create_dynaconf_with_custom_loader to prevent circular import errors.

tests/unittest/test_fresh_vars_functionality.py [150-168]

     def test_gitlab_personal_access_token_reload(self):
         """
         Test that gitlab.personal_access_token is reloaded when marked as fresh.
 
         This is the critical test for the user's use case. It verifies that:
         1. Initial value is loaded correctly
         2. After modifying the file, the new value is returned (not cached)
         3. This works with the custom_merge_loader
         """
         # Create initial secrets file
         self.create_secrets_toml(personal_access_token="token_v1", shared_secret="secret_v1")
 
         # Create Dynaconf with GITLAB marked as fresh
-        settings = create_dynaconf_with_custom_loader(self.temp_dir, self.secrets_file, fresh_vars=["GITLAB"])
+        settings = self.create_dynaconf_with_custom_loader(fresh_vars=["GITLAB"])
 
         # First access - should return initial value
         first_token = settings.GITLAB.PERSONAL_ACCESS_TOKEN
         assert first_token == "token_v1", "Initial personal_access_token should be 'token_v1'"
 ...

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical bug where tests in TestFreshVarsGitLabScenario call a global helper instead of the required class method, which would lead to test failures due to circular imports.

High
Fix broken file timestamp update

Uncomment the os.utime(...) call in the ensure_file_timestamp_changes function
to ensure it correctly updates file timestamps, which is critical for the
reliability of the configuration reloading tests.

tests/unittest/test_fresh_vars_functionality.py [28-44]

     def ensure_file_timestamp_changes(file_path):
         """
         Force file timestamp to change by using os.utime with explicit time.
     
         This is much faster than sleeping and works on all filesystems.
     
         Args:
             file_path: Path to the file to update
         """
         # Get current time and add a small increment to ensure it's different
         current_time = time.time()
         new_time = current_time + 0.001  # Add 1ms
     
         # Set both access time and modification time
-        # os.utime(file_path, (new_time, new_time))
+        os.utime(file_path, (new_time, new_time))

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that the os.utime call is commented out, making the ensure_file_timestamp_changes function ineffective and potentially causing the tests to be unreliable.

High
Learned
best practice
Add exception handling in teardown

Wrap the cleanup operation in a try-except block to handle potential permission
or file system errors gracefully, preventing test teardown failures from masking
actual test failures.

tests/unittest/test_fresh_vars_functionality.py [93-98]

 def teardown_method(self):
     """Clean up temporary files after each test."""
     import shutil
 
     if hasattr(self, "temp_dir") and Path(self.temp_dir).exists():
-        shutil.rmtree(self.temp_dir)
+        try:
+            shutil.rmtree(self.temp_dir)
+        except Exception as e:
+            # Log but don't fail teardown
+            print(f"Warning: Failed to clean up temp dir: {e}")
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Improve exception handling by preserving context (raise from), capturing exceptions into variables, sanitizing logs to avoid secrets leakage, and converting returns of exception objects into proper raises.

Low
  • Update
  • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

@pdecat
Copy link
Contributor Author

pdecat commented Nov 14, 2025

Second test workflow with the fix:

================= 199 passed, 6 skipped, 24 warnings in 6.17s ==================

https://github.com/qodo-ai/pr-agent/actions/runs/19370355959/job/55424387444?pr=2104#step:5:302

@pdecat pdecat force-pushed the fix/dynaconf_fresh_vars branch from 312134a to 4f5e1f7 Compare November 14, 2025 16:19
@pdecat
Copy link
Contributor Author

pdecat commented Nov 14, 2025

Note: I cannot check the boxes in the qodo-merge-for-open-source comment, they are disabled:
image

@maxdata
Copy link

maxdata commented Nov 15, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Fresh Vars Matching

The fresh_vars comparison uses case-insensitive matching via key.upper() == k.upper(). Confirm that this aligns with Dynaconf's expectations and does not unintentionally match similarly named keys differing only by case across environments or nested structures.

# For fresh_vars support: key parameter is uppercase, but accumulated_data keys are lowercase
if key is None or key.upper() == k.upper():
    obj.set(k, v)
Test Robustness

Tests rely on os.utime with a 1ms increment to trigger reloads. On some filesystems timestamp granularity may be coarser; consider increasing the delta or adding a fallback sleep to avoid flakes in CI.

def ensure_file_timestamp_changes(file_path):
    """
    Force file timestamp to change by using os.utime with explicit time.

    This is much faster than sleeping and works on all filesystems.

    Args:
        file_path: Path to the file to update
    """
    # Get current time and add a small increment to ensure it's different
    current_time = time.time()
    new_time = current_time + 0.001  # Add 1ms

    # Set both access time and modification time
    os.utime(file_path, (new_time, new_time))
Env Var Scope

Only one test sets FRESH_VARS_FOR_DYNACONF via patch.dict; ensure no leakage between tests and consider adding a negative test confirming behavior when the env var is absent or malformed.

# Set FRESH_VARS_FOR_DYNACONF environment variable
with patch.dict(os.environ, {"FRESH_VARS_FOR_DYNACONF": '["GITLAB"]'}):
    # Create Dynaconf (should pick up fresh_vars from env)
    settings = create_dynaconf_with_custom_loader(self.temp_dir, self.secrets_file)

    # First access
    first_value = settings.GITLAB.PERSONAL_ACCESS_TOKEN
    assert first_value == "env_token_v1"

    # Modify file
    ensure_file_timestamp_changes(self.secrets_file)
    self.create_secrets_toml(personal_access_token="env_token_v2")

    # Second access - should be reloaded
    second_value = settings.GITLAB.PERSONAL_ACCESS_TOKEN
    assert second_value == "env_token_v2", "fresh_vars from environment variable should cause reload"

    assert first_value != second_value, "Values should be different when fresh_vars is set via env var"

@ofir-frd
Copy link
Collaborator

@sharoneyal please have a look here.

@Abdulwahid84
Copy link

🤖 Automated PR Review

Summary

✅ Code review complete. Analyzed 2 files. No major issues found. The code looks good!

✅ No issues found!


Generated by AI-powered PR Review Agent

@Abdulwahid84
Copy link

🤖 Automated PR Review

Summary

Code Review Complete
PR #2104: fix: restore Dynaconf fresh vars support
Analyzed: 2 files
Issues Found: 0 total
✅ No major issues detected. Code looks good!

Detailed Findings

📄 pr_agent/custom_merge_loader.py

📄 tests/unittest/test_fresh_vars_functionality.py


Generated by AI-powered PR Review Agent

4 similar comments
@Abdulwahid84
Copy link

🤖 Automated PR Review

Summary

Code Review Complete
PR #2104: fix: restore Dynaconf fresh vars support
Analyzed: 2 files
Issues Found: 0 total
✅ No major issues detected. Code looks good!

Detailed Findings

📄 pr_agent/custom_merge_loader.py

📄 tests/unittest/test_fresh_vars_functionality.py


Generated by AI-powered PR Review Agent

@Abdulwahid84
Copy link

🤖 Automated PR Review

Summary

Code Review Complete
PR #2104: fix: restore Dynaconf fresh vars support
Analyzed: 2 files
Issues Found: 0 total
✅ No major issues detected. Code looks good!

Detailed Findings

📄 pr_agent/custom_merge_loader.py

📄 tests/unittest/test_fresh_vars_functionality.py


Generated by AI-powered PR Review Agent

@Abdulwahid84
Copy link

🤖 Automated PR Review

Summary

Code Review Complete
PR #2104: fix: restore Dynaconf fresh vars support
Analyzed: 2 files
Issues Found: 0 total
✅ No major issues detected. Code looks good!

Detailed Findings

📄 pr_agent/custom_merge_loader.py

📄 tests/unittest/test_fresh_vars_functionality.py


Generated by AI-powered PR Review Agent

@Abdulwahid84
Copy link

🤖 Automated PR Review

Summary

Code Review Complete
PR #2104: fix: restore Dynaconf fresh vars support
Analyzed: 2 files
Issues Found: 0 total
✅ No major issues detected. Code looks good!

Detailed Findings

📄 pr_agent/custom_merge_loader.py

📄 tests/unittest/test_fresh_vars_functionality.py


Generated by AI-powered PR Review Agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants