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

Fixed continuous config appending #83

Merged
merged 1 commit into from
Jul 26, 2017
Merged

Conversation

duanehutchins
Copy link
Contributor

Fixed the issue where the stunnel-pgbouncer.conf, user.txt and pgbouncer.ini files are being repeatedly appended on server restarts, instead of erasing and restarting.
Also fixed the DATABASE_URL parse to not fail on missing passwords.

@edmorley
Copy link
Member

On Heroku the filesystem is ephemeral, so previous run's configs won't exist any more.
Was the issue you were seeing when using this buildpack ion other environments?

@edmorley
Copy link
Member

Ah there's a dupe of this, #70 which gives an example scenario.

Copy link
Contributor

@gregburek gregburek left a comment

Choose a reason for hiding this comment

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

The change described in the title is fine, but I am suspicious of the change in connection string parsing. As we don't have tests for this, I don't know how this was validated or how it would work with odd input.

I suggest breaking that change into another pr so we can test and qualify it without blocking the config appending.

@duanehutchins
Copy link
Contributor Author

@gregburek I split this into two separate pull requests.

@gregburek gregburek merged commit 948fa64 into heroku:master Jul 26, 2017
gregburek added a commit that referenced this pull request Jul 26, 2017
The users.txt file must be appended to as multiple postgres dbs in
POSTGRES_URLS, we need to add all the creds to the users.txt file, not
reset it for every entry in POSTGRES_URLS.
@@ -86,7 +86,7 @@ connect = $DB_HOST:$DB_PORT
retry = ${PGBOUNCER_CONNECTION_RETRY:-"no"}
EOFEOF

cat >> /app/vendor/pgbouncer/users.txt << EOFEOF
cat > /app/vendor/pgbouncer/users.txt << EOFEOF
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is inside the for loop, so it must be append only. Fixed in #89

gregburek added a commit that referenced this pull request Jul 26, 2017
The users.txt file must be appended to as multiple postgres dbs in
POSTGRES_URLS, we need to add all the creds to the users.txt file, not
reset it for every entry in POSTGRES_URLS.
cyberdelia pushed a commit to heroku/heroku-buildpack-redis that referenced this pull request Sep 11, 2017
Otherwise running `start-stunnel <COMMAND>` multiple times (such as
in a one-off dyno when running a command and then killing it) will
result in an stunnel.conf that has invalid syntax due to the global
options being appended after the per-server header.

Based on:
heroku/heroku-buildpack-pgbouncer#83
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

3 participants