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

[misc] Add support for NETBIRD_STORE_ENGINE_POSTGRES_DSN environment variable in setup.env #2462

Merged
merged 4 commits into from
Aug 23, 2024

Conversation

arosberg
Copy link
Contributor

Describe your changes

Updated the configure script and docker-compose templates to better support deployment with postgres. Prevents the need to manually add the NETBIRD_STORE_ENGINE_POSTGRES_DSN environment variable to the docker-compose file after the configuration script is run (https://docs.netbird.io/selfhosted/postgres-store). Also provides warning to the user if the NETBIRD_STORE_CONFIG_ENGINE=postgres and NETBIRD_STORE_ENGINE_POSTGRES_DSN is not set.

Issue ticket number and link

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@CLAassistant
Copy link

CLAassistant commented Aug 21, 2024

CLA assistant check
All committers have signed the CLA.

@mlsmaycon
Copy link
Collaborator

Hello @arosberg, thanks for the contribution. Can you add tests to validate the environment variable setting?

You can check these files for that:
https://github.com/netbirdio/netbird/blob/c61cb00f40a2b94c27932312535bfd8498f4757a/infrastructure_files/tests/setup.env

@mlsmaycon mlsmaycon changed the title Add support for NETBIRD_STORE_ENGINE_POSTGRES_DSN environment variable in setup.env [misc] Add support for NETBIRD_STORE_ENGINE_POSTGRES_DSN environment variable in setup.env Aug 22, 2024
@arosberg
Copy link
Contributor Author

Hello @mlsmaycon

The way I wrote the script the environment variable will only be exported if NETBIRD_STORE_CONFIG_ENGINE=postgres and in the test cases NETBIRD_STORE_CONFIG_ENGINE=sqlite so it will not be exported by the script (And therefore not substituted in the compose template). One thing I could do is always export the NETBIRD_STORE_ENGINE_POSTGRES_DSN variable by adding it to the base.setup.env exports

# exports
export NETBIRD_DOMAIN
export NETBIRD_TURN_DOMAIN
export NETBIRD_AUTH_CLIENT_ID
export NETBIRD_AUTH_CLIENT_SECRET

Then it would substitute even if NETBIRD_STORE_CONFIG_ENGINE=sqlite (Then I would be able to add test cases). Another option is adding a new test just for postgres but this seems a bit heavy to me. What do you think?

Copy link

@mlsmaycon
Copy link
Collaborator

Hello @mlsmaycon

The way I wrote the script the environment variable will only be exported if NETBIRD_STORE_CONFIG_ENGINE=postgres and in the test cases NETBIRD_STORE_CONFIG_ENGINE=sqlite so it will not be exported by the script (And therefore not substituted in the compose template). One thing I could do is always export the NETBIRD_STORE_ENGINE_POSTGRES_DSN variable by adding it to the base.setup.env exports

# exports
export NETBIRD_DOMAIN
export NETBIRD_TURN_DOMAIN
export NETBIRD_AUTH_CLIENT_ID
export NETBIRD_AUTH_CLIENT_SECRET

Then it would substitute even if NETBIRD_STORE_CONFIG_ENGINE=sqlite (Then I would be able to add test cases). Another option is adding a new test just for postgres but this seems a bit heavy to me. What do you think?

that's true, we will add a matrix and a service for that.

@mlsmaycon mlsmaycon merged commit 33b264e into netbirdio:main Aug 23, 2024
25 checks passed
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.

3 participants