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

adding in a generated default DB port. #469

Merged
merged 1 commit into from
Mar 18, 2020
Merged

Conversation

jwoertink
Copy link
Member

In the PR luckyframework/avram#333 we are removing the default port value to fix some other issues with connecting to sockets. This PR adds in the port default to your new generated lucky app.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

I think this answers the question I just left on the other PR :P

Is there some way we could set the default port when needed in Avram? Like only if you’re not using a socket or something? I don’t really understand the other change so maybe I’m just spouting nonsense :P

@jwoertink
Copy link
Member Author

We were force specifying the default port before, but I'm pretty sure postgres will just fallback to it's default without specifying anyway, so this change isn't totally required. This just moves that setting down to the user level so people have the option to not use it if they need. Otherwise this should all work as it does now.

@paulcsmith
Copy link
Member

paulcsmith commented Mar 17, 2020 via email

@jwoertink
Copy link
Member Author

Yeah, passing in the port isn't necessary if postgres is running on the default port. And really we're just talking about this update for development, so production isn't affected at all. The main benefit here is just that if someone needs to change the port locally, it's a lot more apparent how and where to do that. (or if using docker for development)

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Sounds good to me! 👍

@jwoertink jwoertink merged commit c2cd4ec into master Mar 18, 2020
@jwoertink jwoertink deleted the updates/default_db_port branch March 18, 2020 00:38
paulcsmith pushed a commit that referenced this pull request Apr 7, 2020
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.

None yet

2 participants