fix: Litestream S3 - add support to deprecated env variables #8555
Conversation
|
Warning Rate Limit Exceeded@pranavxc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 0 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/docker/start-litestream.sh (2 hunks)
Additional Context Used
ShellCheck (2)
packages/nocodb/docker/start-litestream.sh (2)
[error] 14-14: To expand via indirection, use arrays, ${!name} or (for sh only) eval.
[warning] 28-28: This } is literal. Check expression (missing ;/\n?) or quote it.
Additional comments not posted (1)
packages/nocodb/docker/start-litestream.sh (1)
28-28: Verify the syntax around the conditional braces.
ad35cb5 to
90008af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/docker/start-litestream.sh (2 hunks)
Additional Context Used
ShellCheck (1)
packages/nocodb/docker/start-litestream.sh (1)
[warning] 28-28: This } is literal. Check expression (missing ;/\n?) or quote it.
Additional comments not posted (1)
packages/nocodb/docker/start-litestream.sh (1)
14-16: The backward compatibility for S3 environment variables is correctly implemented.
|
Uffizzi Preview |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/docker/start-litestream.sh (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/nocodb/docker/start-litestream.sh
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/nocodb/docker/litestream.ymlis excluded by!**/*.yml
Files selected for processing (1)
- packages/nocodb/docker/start-litestream.sh (2 hunks)
Additional Context Used
ShellCheck (6)
packages/nocodb/docker/start-litestream.sh (6)
[info] 22-22: The mentioned syntax error was in this if expression.
[error] 22-22: You need a space after the [ and before the ].
[error] 22-22: Couldn't parse this test expression. Fix to allow more checks.
[error] 22-22: Expected this to be an argument to the unary condition.
[error] 22-22: You need a space before the ].
[error] 22-22: Missing space before ]. Fix any mentioned problems and try again.
Additional comments not posted (3)
packages/nocodb/docker/start-litestream.sh (3)
17-19: Good use of conditional checks to ensure backward compatibility with deprecated environment variables.
21-21: SettingLITESTREAM_S3_FORCE_PATH_STYLEto "false" by default is a sensible default, ensuring compatibility with most S3 configurations.
14-16: Ensure thatAWS_BUCKET_PATHis always defined before use.
d8bdb59 to
6b9ffa6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/nocodb/docker/litestream.ymlis excluded by!**/*.yml
Files selected for processing (1)
- packages/nocodb/docker/start-litestream.sh (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/nocodb/docker/start-litestream.sh
01da820 to
646766d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
packages/nocodb/Dockerfile (4)
Line range hint
10-10: Pin package versions in apk add commands to ensure reproducibility.- RUN apk add --no-cache git make musl-dev gcc + RUN apk add --no-cache git=2.30.2-r0 make=4.3-r0 musl-dev=1.2.2-r3 gcc=10.2.1_pre1-r3
Line range hint
13-15: Consolidate consecutive RUN instructions to reduce layers and improve build efficiency.- RUN git clone https://github.com/benbjohnson/litestream.git litestream - RUN cd litestream && go install ./cmd/litestream - RUN cp $GOPATH/bin/litestream /usr/src/lt + RUN git clone https://github.com/benbjohnson/litestream.git litestream && \ + cd litestream && \ + go install ./cmd/litestream && \ + cp $GOPATH/bin/litestream /usr/src/lt
Line range hint
24-24: Pin package versions in apk add commands to ensure reproducibility.- RUN apk add --no-cache python3 make g++ + RUN apk add --no-cache python3=3.9.5-r1 make=4.3-r0 g++=10.2.1_pre1-r3
Line range hint
63-63: Pin package versions in apk add commands to ensure reproducibility.- RUN apk add --update --no-cache dasel dumb-init nodejs + RUN apk add --update --no-cache dasel=1.20.0-r0 dumb-init=1.2.5-r0 nodejs=14.17.0-r0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/nocodb/docker/litestream.ymlis excluded by!**/*.yml
Files selected for processing (3)
- packages/noco-docs/docs/020.getting-started/050.self-hosted/020.environment-variables.md (1 hunks)
- packages/nocodb/Dockerfile (1 hunks)
- packages/nocodb/docker/start-litestream.sh (2 hunks)
Additional Context Used
LanguageTool (1)
packages/noco-docs/docs/020.getting-started/050.self-hosted/020.environment-variables.md (1)
Near line 25: Unpaired symbol: ‘'’ seems to be missing
Context: ...ter. Allowed special characters include$&+,:;=?@#\|'.^*()%!_-". | ...
ShellCheck (2)
packages/nocodb/docker/start-litestream.sh (2)
[warning] 14-14: This flag is used as a command name. Bad line break or missing [ .. ]?
[warning] 14-14: Found trailing ] outside test. Add missing [ or quote if intentional.
Hadolint (6)
packages/nocodb/Dockerfile (6)
10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
13: Multiple consecutive
RUNinstructions. Consider consolidation.
14: Use WORKDIR to switch to a directory
15: Double quote to prevent globbing and word splitting.
24: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
63: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
Additional comments not posted (1)
packages/nocodb/docker/start-litestream.sh (1)
36-38: Ensure default values are set correctly in the script to avoid unexpected behavior.Setting default values like this ensures that the system behaves predictably even when environment variables are not explicitly set by the user.
fee20da to
bebff78
Compare
Signed-off-by: Pranav C <pranavxc@gmail.com>
bebff78 to
f441403
Compare
| @@ -19,14 +26,16 @@ use_litestream() { | |||
| && [ -z "${DATABASE_URL}" ] \ | |||
| && [ -z "${DATABASE_URL_FILE}" ] \ | |||
| && [ -z "${NC_MINIMAL_DBS}" ] \ | |||
| && [ -n "${LITESTREAM_S3_ENDPOINT}" ] \ | |||
There was a problem hiding this comment.
@pranavxc You should rather check whether either one of LITESTREAM_S3_ENDPOINT or LITESTREAM_S3_REGION is set than just deleting this line...
There was a problem hiding this comment.
It fallbacks to the default region or endpoint if not set so kept as not mandatory
There was a problem hiding this comment.
What are the default region and endpoint? Where are they defined?I didn't spot anything about this in the Litestream doc...
There was a problem hiding this comment.
Even I couldn’t find that details but it fallbacks to us-east-1 I guess
There was a problem hiding this comment.
Hm, ok. We should then update the env var documentation if we keep it that way.
There was a problem hiding this comment.
I didn’t added that info since I couldn’t find that info anywhere in their docs
| # set default bucket path if not provided | ||
| : "${LITESTREAM_S3_PATH:=nocodb}" |
There was a problem hiding this comment.
To be consistent, I would rather set this in the Dockerfile where the other Litestream-specific defaults are set.
There was a problem hiding this comment.
If we set it at docker file it's hard to add backward compatibility for env var AWS_BUCKET_PATH
There was a problem hiding this comment.
Ah, I see. Then I guess we have to leave it like this.
| @@ -6,6 +6,7 @@ dbs: | |||
| - type: s3 | |||
| endpoint: ${LITESTREAM_S3_ENDPOINT} | |||
| force-path-style: true | |||
| region: ${LITESTREAM_S3_REGION} | |||
There was a problem hiding this comment.
@pranavxc I think setting region does only make sense when not setting endpoint at the same time since endpoint already includes the region, cf. https://litestream.io/reference/config/#s3-replica.
Did you test what happens if LITESTREAM_S3_REGION is unset and LITESTREAM_S3_ENDPOINT is set and vice versa? Does Litestream ignore empty config entries?
There was a problem hiding this comment.
I tested and it was fine, will test again to confirm
There was a problem hiding this comment.
Note that I hardcoded force-path-style: true in the Dockerfile start script.
About force-path-style:
Uses the path style which is required by non-AWS services. This is automatically enabled if
endpointis set.
So I guess we'd have to remove force-path-style: true if we wanna support region, no?
(In the end I don't think it's really worth to support region since it only brings a tiny bit of convenience for AWS and Backblaze users but complicates support for other S3-compatible hosters.)
There was a problem hiding this comment.
Even with force-path-style: region was working for me. I kept it because setting region is easier than figuring out the endpoint.
There was a problem hiding this comment.
Even with
force-path-style:region was working for me.
Ok, interesting...
I kept it because setting region is easier than figuring out the endpoint.
I see^^
I still think it would be best to adhere to what the official Litestream doc says, hence remove force-path-style: true if we keep region :)
There was a problem hiding this comment.
Ya better to remove since it automatically going to set to true when we are configuring custom endpoint
| export LITESTREAM_S3_PATH="${AWS_BUCKET_PATH}" | ||
| fi | ||
| if [ -z "${LITESTREAM_S3_BUCKET}" ] && [ -n "${AWS_BUCKET}" ] ; then | ||
| export LITESTREAM_S3_BUCKET="${AWS_BUCKET}" |
There was a problem hiding this comment.
I guess, we should also document that AWS_BUCKET_PATH and AWS_BUCKET are deprecated, same as for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY.
There was a problem hiding this comment.
Agree and thanks for pointing out.
| @@ -11,6 +11,13 @@ fi | |||
| if [ -z "${LITESTREAM_S3_SECRET_ACCESS_KEY}" ] && [ -n "${AWS_SECRET_ACCESS_KEY}" ] ; then | |||
| export LITESTREAM_S3_SECRET_ACCESS_KEY="${AWS_SECRET_ACCESS_KEY}" | |||
| fi | |||
| if [ -z "${LITESTREAM_S3_PATH}" ] && -n [ "${AWS_BUCKET_PATH}" ] ; then | |||
There was a problem hiding this comment.
@pranavxc This seems to be invalid syntax, the -n test needs to be inside the brackets.
There was a problem hiding this comment.
Possibly I will create a follow up PR with your suggested changes.
There was a problem hiding this comment.
Could you ping me when you submit that follow-up PR? 🙂
There was a problem hiding this comment.
In between something, so once sorted out, I will work on it. I’m happy if you would like to open a PR as well.
There was a problem hiding this comment.
I've just submitted #8572 which should address everything we discussed here (except for testing) :)
There was a problem hiding this comment.
Thanks for creating the PR, soon I will test it and merge.
Change Summary
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of