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

Make Litestream retention configurable #447

Merged
merged 5 commits into from
Jul 19, 2023
Merged

Make Litestream retention configurable #447

merged 5 commits into from
Jul 19, 2023

Conversation

ribtoks
Copy link
Contributor

@ribtoks ribtoks commented Jul 18, 2023

Details in #446

One problem though: due to current code organization (few "entrypoints"), I did not find a good single place for a default value.

@github-actions
Copy link

github-actions bot commented Jul 18, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@ribtoks
Copy link
Contributor Author

ribtoks commented Jul 18, 2023

I have read the CLA Document and I hereby sign the CLA

@ribtoks
Copy link
Contributor Author

ribtoks commented Jul 18, 2023

recheck

Copy link
Owner

@mtlynch mtlynch left a comment

Choose a reason for hiding this comment

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

I have read the CLA Document and I hereby sign the CLA

It looks like the email address you're using for commits (the @mailbox.org address) isn't associated with your Github account, so CLAbot won't accept your signature.

Can you add the email address to your account? Or add a commit using an email address that's already associated with your Github account?

https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user

README.md Outdated
@@ -59,6 +59,7 @@ LITESTREAM_BUCKET=YOUR-LITESTREAM-BUCKET
LITESTREAM_ENDPOINT=YOUR-LITESTREAM-ENDPOINT
LITESTREAM_ACCESS_KEY_ID=YOUR-ACCESS-ID
LITESTREAM_SECRET_ACCESS_KEY=YOUR-SECRET-ACCESS-KEY
LITESTREAM_RETENTION="72h" # optional
Copy link
Owner

Choose a reason for hiding this comment

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

We can omit this here, since I want to limit it to the simplest set of options.

README.md Outdated
@@ -123,6 +124,7 @@ You can adjust behavior of the Docker container by specifying these Docker-speci
| `LITESTREAM_ENDPOINT` | Litestream-compatible cloud storage endpoint where Litestream should replicate data. |
| `LITESTREAM_ACCESS_KEY_ID` | Litestream-compatible cloud storage access key ID to the bucket where you want to replicate data. |
| `LITESTREAM_SECRET_ACCESS_KEY` | Litestream-compatible cloud storage secret access key to the bucket where you want to replicate data. |
| `LITESTREAM_RETENTION` | The amount of time Litestream snapshots & WAL files will be kept. (default 72h) |
Copy link
Owner

Choose a reason for hiding this comment

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

Can we say "(defaults to 72h)." to match the conventions above?

(add 'to' and period after parens)

@@ -23,6 +23,7 @@ set +x
set -x

PS_SHARED_SECRET=somepassword
LITESTREAM_RETENTION=${LITESTREAM_RETENTION:-72h}
Copy link
Owner

Choose a reason for hiding this comment

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

We can skip the retention setting in this file, as this script is just for development.

@@ -77,6 +77,8 @@ LITESTREAM_BUCKET="my-pico-bucket"
LITESTREAM_ENDPOINT="s3.us-west-002.backblazeb2.com"
```

Additionally you can set up "retention" (see [Litestream replica settings](https://litestream.io/reference/config/#replica-settings)) for how long snapshots will be kept, using `LITESTREAM_RETENTION` variable. If unset, default value of `72h` will be used.
Copy link
Owner

Choose a reason for hiding this comment

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

We can leave out the documentation here for simplicity. Otherwise, we end up redundantly defining every env variable in every deployment doc.

github-actions bot added a commit that referenced this pull request Jul 19, 2023
@ribtoks
Copy link
Contributor Author

ribtoks commented Jul 19, 2023

recheck

@ribtoks
Copy link
Contributor Author

ribtoks commented Jul 19, 2023

e2e tests are a bit flaky, I had to bump the playwright timeout (I understand it's an "unrelated change")

Copy link
Owner

@mtlynch mtlynch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for implementing this!

@mtlynch mtlynch merged commit e3ba3a7 into mtlynch:master Jul 19, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants