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

Split host and port in .env variables #1363

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

Conversation

jerryfletcher21
Copy link
Contributor

@jerryfletcher21 jerryfletcher21 commented Jul 4, 2024

What does this PR do?

Split host and port in .env variables.
Related to #1364.

Checklist before merging

  • Install pre-commit and initialize it: pip install pre-commit, then pre-commit install. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.

Copy link
Member

@KoalaSat KoalaSat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice proposal! We shouldn't forget about double check if there is any effect on https://github.com/RoboSats/robosats-deploy, since its the software that coordinators run

@jerryfletcher21
Copy link
Contributor Author

@KoalaSat this PR is related to #1364, but I split them in two different PR to better manage them. Ah yes thank you I did not think about robosats-deploy, I will have to check it.

@jerryfletcher21
Copy link
Contributor Author

@KoalaSat I have made a PR also in robosats-deploy, thank you for your feedback!

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented Jul 7, 2024

If we want to add new variables for host/port split into two vars, we need to keep supporting the current variable names with deprecation notice. Re-factoring the .env can lead to production issues to the coordinators as they need to conciously go and edit their .env (in their robosats-deploy) at the moment they pull the new robosats coordinator image.

Creating a PR in robosats-deploy is not enough: we have to document it (how an existing .env must be edited) and make sure all coordinators are aware before releasing a robosats-coordinator image with this change.

What is the motivation for splitting host and port into two variables? I agree it's cleaner, but not sure if worth the amount of trouble this upgrade can potentially cause.

@jerryfletcher21
Copy link
Contributor Author

@Reckless-Satoshi yes the coordinators would have to change their .env files and I agree it may lead to production issues if they are not informed correctly.
I made this PR because while developing #1364, I needed to get just the port and not the host, and also because postgres variables in .env are already separated in host and port, so it is cleaner and more modular to have them all separated.
I could rewrite this PR to still support the old variables if the new ones are not present, or if you do not want the trouble this PR may cause I can simply extract the ports from the variables in #1364 and not base it on this PR.

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