Skip to content
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

Add config/appsignal.rb config file #1324

Merged
merged 5 commits into from
Nov 11, 2024
Merged

Add config/appsignal.rb config file #1324

merged 5 commits into from
Nov 11, 2024

Commits on Nov 5, 2024

  1. Add config/appsignal.rb config file

    Add `config/appsignal.rb` config file support. This is a replacement for
    the `config/appsignal.yml` config file.
    
    Load the `config/appsignal.rb` file automatically on `Appsignal.start`.
    For most apps, like Rails apps, this will work the same way as
    `config/appsignal.yml` does now.
    
    If `Appsignal.configure` is called manually in the app, like a Rails
    initializer, the `config/appsignal.rb` file is not loaded. Instead, we
    recommend moving the `Appsignal.configure` call to the
    `config/appsignal.rb` file.
    
    ## New behavior
    
    With this change the config is loaded as described in these scenarios:
    
    - Scenario 1:
      - `Appsignal.start` is called.
      - No config is loaded yet.
      - The `config/appsignal.rb` file is present.
      - The `config/appsignal.rb` is required and configures the gem.
    - Scenario 2:
      - `Appsignal.start` is called.
      - No config is loaded yet.
      - The `config/appsignal.rb` file is present.
      - The `config/appsignal.yml` file is present.
      - The `config/appsignal.rb` is required and configures the gem using
        the `Appsignal.configure` helper.
      - The `config/appsignal.yml` is not loaded.
    - Scenario 3:
      - `Appsignal.configure` is called.
      - The `config/appsignal.rb` file is present.
      - The `config/appsignal.rb` file is not loaded.
      - `Appsignal.start` is called.
    - Scenario 4:
      - `Appsignal.configure` is called.
      - The `config/appsignal.yml` file is present.
      - The `config/appsignal.yml` file is loaded.
      - `Appsignal.start` is called.
    
    This is more complex that I would have liked. In hindsight I shouldn't
    have made `Appsignal.configure` load the `config/appsignal.yml` file.
    That would have made it a bit more predictable. But I don't want
    `Appsignal.configure` in the `config/appsignal.rb` file to also load the
    `config/appsignal.yml` file, because then it just gets more confusing.
    
    In the next major version I want to remove `config/appsignal.yml`
    support, so this is the first step.
    
    ## App/root path
    
    I remember some of our users configured the Config class's `root_path`
    to specify where to load the config file from.
    If an app configures itself via a `config/appsignal.rb`, this makes it
    impossible to customize the root path. For that purpose I've added the
    `APPSIGNAL_APP_PATH` environment variable to customize the config path.
    Which I've also used to make it easier to test the different scenarios.
    
    ---
    
    Part of #1324
    tombruijn committed Nov 5, 2024
    Configuration menu
    Copy the full SHA
    f81248b View commit details
    Browse the repository at this point in the history
  2. Move config load logic to its own method

    Add a private method (that has to be publicly accessible within the gem
    itself), to load the config. This avoids duplication in the diagnose CLI
    when I update it to call this method as well.
    
    Make it more safe by replicating the existing behavior: it will never
    return a `nil` object for the Config once started. If it started with an
    error or the config file is empty, initialize the config but set
    `active` to `false`.
    tombruijn committed Nov 5, 2024
    Configuration menu
    Copy the full SHA
    5c45e97 View commit details
    Browse the repository at this point in the history
  3. Load config/appsignal.rb in diagnose CLI

    Update the diagnose CLI to call the new `Appsignal._load_config!`
    PRIVATE method to initialize the config. This way we have less
    duplication of how the config is initialized and we can be sure it works
    the same in the diagnose CLI as well.
    
    I've introduced a new PRIVATE `_APPSIGNAL_CONFIG_FILE_ENV` env var to
    set the environment, but only in the diagnose CLI context. It's one of
    the few ways we can pass the default environment as set by the CLI to
    whatever config object gets initialized in the config file.
    
    I didn't use the `APPSIGNAL_APP_ENV` environment variable so the config
    option doesn't show up in the report as being set by an env var or
    overwrite it when the user has set it.
    tombruijn committed Nov 5, 2024
    Configuration menu
    Copy the full SHA
    59f8283 View commit details
    Browse the repository at this point in the history

Commits on Nov 11, 2024

  1. Improve changeset copy for appsignal.rb file

    These are suggestions from the review and my own improvements.
    tombruijn committed Nov 11, 2024
    Configuration menu
    Copy the full SHA
    48d16c7 View commit details
    Browse the repository at this point in the history
  2. Add warnings for config file behaviors

    Add warnings to stderr and the log file when some config combination is
    used that may be expected to work, but does not. This is in preparation
    of the YAML file removal and to encourage apps to use the
    `config/appsignal.rb` config file and/or `Appsignal.configure` helper.
    tombruijn committed Nov 11, 2024
    Configuration menu
    Copy the full SHA
    cdbec61 View commit details
    Browse the repository at this point in the history