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 a --test-db global parameter to use TEST_DATABAE_URL env instead #4201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

urkle
Copy link

@urkle urkle commented Aug 24, 2024

The use case here is I have my local development setup where I have two databases setup, development and testing.

my .env has

DATABASE_URL=dev_url
TEST_DATABASE_URL=test_url

Currently I have a wrapper diesel-test.sh script that load the .env and switches DATABASE_URL with TEST_DATABASE_URL's contents. However this interrupts the code-completion in my shell when running changes against my test DB.

This PR adds a --test-db (-t) global parameter that will do that switch for me in the cli so I can easily switch between the two environments on the command line and still have code-completion easily.

diesel -t migration run

Outstanding issues/questions

  • Can the diesel_cli::errors::Error enum be modified to allow the DatabaseUrlMissing enum to take a parameter of the ENV name? Or add a second enum for the Test URL message variant?
    #[error("The --database-url argument must be passed, or the {} environment variable must be set.")]
     DatabaseUrlMissing(String),
    or
    #[error("The --database-url argument must be passed, or the DATABASE_URL environment variable must be set.")]
     DatabaseUrlMissing,
    #[error("The --database-url argument must be passed, or the TEST_DATABASE_URL environment variable must be set.")]
     TestDatabaseUrlMissing,
    • Also shouldn't this enum be non_exhaustive?
  • Right now I have it so if you specify --test-db it will NOT look at DATABASE_URL (to prevent unintended behavior if it fell back to DATABASE_URL). This seems a reasonable behavior.

…DATABASE_URL env instead of DATABASE_URL env
@weiznich
Copy link
Member

Thanks for opening this PR.
I'm not sure if that's the best solution as this command line flag sounds like something that fits mostly your workflow, but I do not see any discussion around whether that might be something that would benefit the majority of diesel users. I personally believe the current environment variable setup is sufficient for most of our users. I would expect that you specify the development/test url in your .env file and otherwise manually specify any overwrites as explicit argument/environment variable. That has the huge advantage that it is always explicit if you touch anything else than the default environment. Maybe you can elaborate why this isn't possible for your setup and why you believe this requires a different solution?

@urkle
Copy link
Author

urkle commented Sep 1, 2024

@weiznich yes I do have my DATABASE_URL setup in a .env file, however that means that will load it for one environment.

That has been the issue is that I sometimes need to switch between multiple environments (the common ones are between dev and testing). Working with other frameworks (such as the infamous ruby on rails) their CLI tools support an "environment" override param to switch between development, test, and production (or any other you create) which allows easy switching between dev & testing to work on the database.

What I have been doing thus far is I have a local script in my project named diesel-test.sh with this contents.

#!/bin/sh

. .env
export DATABASE_URL=$TEST_DATABASE_URL

diesel "$@"

However this did not have tab-completion thus making it harder to use on the command line (I live on the CLI).

Digging in some more I found an alternative that accomplishes the same as what this PR provides by doing this.

  1. create a diesel-test script in my ~/bin (in my $PATH)
    #!/bin/bash
    
    if [ ! -f .env ]; then
        echo "Run in the directory containing a .env with a TEST_DATABASE_URL env"
        exit 1
    fi
    
    . .env
    
    if [ -z "$TEST_DATABASE_URL" ]; then
        echo "No TEST_DATABASE_URL defined in .env"
        exit 1
    fi
    
    export DATABASE_URL=$TEST_DATABASE_URL
    
    diesel "$@"
  2. in my bash_completion setup I added this line to configure this new script to have the same alises
    complete -F _diesel -o nosort -o bashdefault -o default diesel-test

I may fiddle more with that diesel-test script as another alternative approach would be to have it load the env from .env.test (e.g. load .env then load .env.test) and having that built-into the diesel cli might be night.. e.g. a -e test or whatever name you want and have it load (via dotenv) the .env.test and the main .env (defaulting to load .env.dev?) this is the way many other tools in other frameworks load things up to allow env-specific override file. (vite does this, php tooling I've used does it, the rails frameworks do.)

@weiznich
Copy link
Member

weiznich commented Sep 2, 2024

Thanks for providing these details. I think I now understand at least the problem that you are trying to solve. I'm still not sold on the implemented approach.

another alternative approach would be to have it load the env from .env.test (e.g. load .env then load .env.test) and having that built-into the diesel cli might be night.. e.g. a -e test or whatever name you want and have it load (via dotenv) the .env.test and the main .env (defaulting to load .env.dev?) this is the way many other tools in other frameworks load things up to allow env-specific override file. (vite does this, php tooling I've used does it, the rails frameworks do.)

That sounds like a much better solution. I would be open to accept a PR that implements such a flag, so that the prefix is used while loading .env files and optionally to prefix the environment variables (so for -e test first try TEST_DATABASE_URL and only if that not exists DATABASE_URL).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants