Skip to content

feat(config): use global config #200

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JungoQuentin
Copy link

@JungoQuentin JungoQuentin commented May 16, 2025

📌 What Does This PR Do?

Add test to cover order of precedence for configuration sources

🔍 Context & Motivation

In order to treat this issue : #191

In order to address this issue (#191), I first added tests to ensure it works as intended

🛠️ Summary of Changes

  • Test: assert order of precedence for configuration sources

📂 Affected Areas

  • Linter
  • Formatter
  • CLI
  • Composer Plugin
  • Dependencies
  • Documentation
  • Other (please specify):

🔗 Related Issues or PRs

related to #191

📝 Notes for Reviewers

@JungoQuentin
Copy link
Author

If I understand the docstring above correctly, the environment variables should be overridden by the other sources. The tests attempt to assert that, but they fail.

If that is the case, I can easily fix it in this PR!

    /// This function attempts to load the configuration from the following sources, in order of precedence:
    ///
    /// 1. A TOML file specified by the `file` argument.
    /// 2. A TOML file named `mago.toml` in the current directory.
    /// 3. Environment variables with the prefix `MAGO_`.

@JungoQuentin JungoQuentin force-pushed the feat/global-config branch 2 times, most recently from 8fd2063 to af13fe2 Compare May 16, 2025 14:13
@azjezz
Copy link
Member

azjezz commented May 16, 2025

If that is the case, I can easily fix it in this PR!

Actually, the doc is wrong.

env vars should always override any other configuration. it is useful to have it this way to allow users to override a specific config for testing purposes, e.g MAGO_THREADS=1 mago lint to check performance under a single thread.

env vars can also be used in CI env to specify configurations that will always apply regardless of the content of the configuration file.

@azjezz azjezz assigned azjezz and JungoQuentin and unassigned azjezz May 16, 2025
@azjezz azjezz added Priority: Medium This issue may be useful and needs attention. Status: Revision Needed Multiple reviewers found issues in the PR that need addressing. Type: Documentation Updating or improving documentation. Type: Maintenance Refactoring, clarifying wording, or removing ambiguity. labels May 16, 2025
@azjezz
Copy link
Member

azjezz commented May 16, 2025

@JungoQuentin could you please fix the documentation to specify that env vars always override other sources, and update the tests accordingly? thank you.

@azjezz
Copy link
Member

azjezz commented May 16, 2025

Priority should ideally be:

  1. env
  2. specified configuration file
  3. configuration file within the current working directory
  4. configuration file in the home directory ( Global mago.toml file #191 )

@JungoQuentin JungoQuentin force-pushed the feat/global-config branch 2 times, most recently from 3563616 to 837ac0d Compare May 17, 2025 17:53
@JungoQuentin
Copy link
Author

Thanks for the clarification @azjezz !

For adding the global configuration file, I propose the following logic:

  • The global configuration overrides the default values.
  • The workspace configuration overrides the default+global values.
  • Using the --file option cancels out what is set in the last two configurations.
  • Environment variables override the values, no matter what.

@JungoQuentin JungoQuentin changed the title test(config): assert order of precedence feat(config): use global config May 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful and needs attention. Status: Revision Needed Multiple reviewers found issues in the PR that need addressing. Type: Documentation Updating or improving documentation. Type: Maintenance Refactoring, clarifying wording, or removing ambiguity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants