You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/22 15:23:33 UTC
[GitHub] [incubator-kvrocks] git-hulk opened a new pull request, #911: Disable the TLS by default
git-hulk opened a new pull request, #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911
This closes #907
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#issuecomment-1255775417
> Do we need to enable TLS in docker images? Currently TLS is enabled in nightly dockerhub images.
Yes, we need to enable TLS in docker image, let me fix it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979215765
##########
.github/workflows/kvrocks.yaml:
##########
@@ -206,7 +206,7 @@ jobs:
run: |
export ${{ matrix.runtime_env_vars }}
./x.py test tcl build --cli-path $HOME/local/bin/redis-cli --dont-clean
- if [[ ! -z "${{ matrix.with_openssl }}" ]] && [[ "${{ matrix.os }}" == ubuntu* ]]; then
+ if [[ ! -z ${{ matrix.with_openssl }} ]] && [[ "${{ matrix.os }}" == ubuntu* ]]; then
Review Comment:
BTW `[[ -n "something" ]]` is another approach to check if it is not empty : )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#issuecomment-1255276510
Do we need to enable TLS in docker images? Currently TLS is enabled in nightly dockerhub images.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979215260
##########
.github/workflows/kvrocks.yaml:
##########
@@ -206,7 +206,7 @@ jobs:
run: |
export ${{ matrix.runtime_env_vars }}
./x.py test tcl build --cli-path $HOME/local/bin/redis-cli --dont-clean
- if [[ ! -z "${{ matrix.with_openssl }}" ]] && [[ "${{ matrix.os }}" == ubuntu* ]]; then
+ if [[ ! -z ${{ matrix.with_openssl }} ]] && [[ "${{ matrix.os }}" == ubuntu* ]]; then
Review Comment:
I think if `matrix.with_openssl` does not exist, it will be replaced to `[[ ! -z ]]` rather than `[[ ! -z "" ]]`, hence maybe it will fail due to syntax error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979259428
##########
.github/workflows/kvrocks.yaml:
##########
@@ -164,10 +174,8 @@ jobs:
- name: Build Kvrocks
run: |
- ./x.py build -j$NPROC --unittest --compiler \
- ${{ matrix.compiler }} ${{ matrix.with_ninja }} ${{ matrix.with_sanitizer }} \
- ${{ matrix.without_jemalloc }} ${{ matrix.without_openssl }} ${{ matrix.without_luajit }} \
- ${{ env.CMAKE_EXTRA_DEFS }}
+ ./x.py build -j$NPROC --unittest --compiler ${{ matrix.compiler }} ${{ matrix.without_jemalloc }} ${{ matrix.without_luajit }} \
Review Comment:
😆 I tried to reorder parameters to work around, but didn't get what's the root cause. Really hope my conclusion is wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979260304
##########
.github/workflows/kvrocks.yaml:
##########
@@ -164,10 +174,8 @@ jobs:
- name: Build Kvrocks
run: |
- ./x.py build -j$NPROC --unittest --compiler \
- ${{ matrix.compiler }} ${{ matrix.with_ninja }} ${{ matrix.with_sanitizer }} \
- ${{ matrix.without_jemalloc }} ${{ matrix.without_openssl }} ${{ matrix.without_luajit }} \
- ${{ env.CMAKE_EXTRA_DEFS }}
+ ./x.py build -j$NPROC --unittest --compiler ${{ matrix.compiler }} ${{ matrix.without_jemalloc }} ${{ matrix.without_luajit }} \
Review Comment:
If it still not work well, I think we can write it in one line, no slash.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979233466
##########
.github/workflows/kvrocks.yaml:
##########
@@ -206,7 +206,7 @@ jobs:
run: |
export ${{ matrix.runtime_env_vars }}
./x.py test tcl build --cli-path $HOME/local/bin/redis-cli --dont-clean
- if [[ ! -z "${{ matrix.with_openssl }}" ]] && [[ "${{ matrix.os }}" == ubuntu* ]]; then
+ if [[ ! -z ${{ matrix.with_openssl }} ]] && [[ "${{ matrix.os }}" == ubuntu* ]]; then
Review Comment:
Thanks @PragmaTwice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r977863116
##########
README.md:
##########
@@ -93,6 +93,13 @@ $ ./x.py build # `./x.py build -h` to check more options;
# especially, `./x.py build --ghproxy` will fetch dependencies via ghproxy.com.
```
+To build with TLS support, you'll need OpenSSL development libraries (e.g. libssl-dev on Debian/Ubuntu) and run:
Review Comment:
Does it sound good to preserve the OpenSSL in prerequisite? I think it's ok since it's a common library and harms nothing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r977831324
##########
README.md:
##########
@@ -93,6 +93,13 @@ $ ./x.py build # `./x.py build -h` to check more options;
# especially, `./x.py build --ghproxy` will fetch dependencies via ghproxy.com.
```
+To build with TLS support, you'll need OpenSSL development libraries (e.g. libssl-dev on Debian/Ubuntu) and run:
Review Comment:
Should we remove openssl packages like `libssl-dev` in Prerequisite above?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979258953
##########
.github/workflows/kvrocks.yaml:
##########
@@ -164,10 +174,8 @@ jobs:
- name: Build Kvrocks
run: |
- ./x.py build -j$NPROC --unittest --compiler \
- ${{ matrix.compiler }} ${{ matrix.with_ninja }} ${{ matrix.with_sanitizer }} \
- ${{ matrix.without_jemalloc }} ${{ matrix.without_openssl }} ${{ matrix.without_luajit }} \
- ${{ env.CMAKE_EXTRA_DEFS }}
+ ./x.py build -j$NPROC --unittest --compiler ${{ matrix.compiler }} ${{ matrix.without_jemalloc }} ${{ matrix.without_luajit }} \
Review Comment:
weird :rofl:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979261007
##########
.github/workflows/kvrocks.yaml:
##########
@@ -164,10 +174,8 @@ jobs:
- name: Build Kvrocks
run: |
- ./x.py build -j$NPROC --unittest --compiler \
- ${{ matrix.compiler }} ${{ matrix.with_ninja }} ${{ matrix.with_sanitizer }} \
- ${{ matrix.without_jemalloc }} ${{ matrix.without_openssl }} ${{ matrix.without_luajit }} \
- ${{ env.CMAKE_EXTRA_DEFS }}
+ ./x.py build -j$NPROC --unittest --compiler ${{ matrix.compiler }} ${{ matrix.without_jemalloc }} ${{ matrix.without_luajit }} \
Review Comment:
ok~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #911: Disable the TLS by default
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #911:
URL: https://github.com/apache/incubator-kvrocks/pull/911#discussion_r979257833
##########
.github/workflows/kvrocks.yaml:
##########
@@ -164,10 +174,8 @@ jobs:
- name: Build Kvrocks
run: |
- ./x.py build -j$NPROC --unittest --compiler \
- ${{ matrix.compiler }} ${{ matrix.with_ninja }} ${{ matrix.with_sanitizer }} \
- ${{ matrix.without_jemalloc }} ${{ matrix.without_openssl }} ${{ matrix.without_luajit }} \
- ${{ env.CMAKE_EXTRA_DEFS }}
+ ./x.py build -j$NPROC --unittest --compiler ${{ matrix.compiler }} ${{ matrix.without_jemalloc }} ${{ matrix.without_luajit }} \
Review Comment:
it seems that some parameters will be lost if there has an extra slash. For example, if `${{ env.CMAKE_EXTRA_DEFS }}` was empty and will become:
```shell
./x.py build -j8 --unittest --compiler \
gcc --ninja \
-DENABLE_OPENSSL=ON \
```
then it causes the TLS server to start failed. cc @PragmaTwice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org