[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42: URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417050675 ## File path: docker/containers/kafka/Dockerfile ## @@ -16,20 +14,4 @@ # See the License for the specific language governing permissions and # limitations under the License. # Review comment: That is fine, maybe a comment in the files stating the reason as you say here? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42:
URL:
https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417046810
##
File path: docker/scripts/docker_run_create_topic_in_kafka.sh
##
@@ -80,7 +80,10 @@ echo "Running docker_run_create_topic_in_kafka with "
echo "NETWORK_NAME = $NETWORK_NAME"
echo "==="
-docker run --rm --network "${NETWORK_NAME}" ches/kafka \
+# TODO: Fix this
+sleep 2s
Review comment:
then, why don't you stick with wait_for_it?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42:
URL:
https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417042597
##
File path: docker/scripts/docker_run_create_topic_in_kafka.sh
##
@@ -80,7 +80,10 @@ echo "Running docker_run_create_topic_in_kafka with "
echo "NETWORK_NAME = $NETWORK_NAME"
echo "==="
-docker run --rm --network "${NETWORK_NAME}" ches/kafka \
+# TODO: Fix this
+sleep 2s
Review comment:
https://docs.docker.com/compose/compose-file/compose-file-v2/#depends_on
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42: URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417041687 ## File path: docker/containers/zookeeper/Dockerfile ## @@ -16,20 +14,4 @@ # See the License for the specific language governing permissions and # limitations under the License. # - -shopt -s nocasematch - -# -# Waits until zookeeper is available on the default port -# - -cd /root || exit 1 - -if [ ! -d /root/scripts ]; then - echo "DOCKER_SCRIPTS_PATH has not been set and mapped" - exit 1 -fi - -cd /root/scripts || exit 1 -bash ./wait-for-it.sh zookeeper:2181 - +FROM zookeeper:3.4 Review comment: same as the other, why do we have this? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42: URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417041583 ## File path: docker/containers/kafka/Dockerfile ## @@ -16,20 +14,4 @@ # See the License for the specific language governing permissions and # limitations under the License. # - -shopt -s nocasematch - -# -# Waits until kafka is available on the default port -# - -cd /root || exit 1 - -if [ ! -d /root/scripts ]; then - echo "DOCKER_SCRIPTS_PATH has not been set and mapped" - exit 1 -fi - -cd /root/scripts || exit 1 -bash ./wait-for-it.sh kafka:9092 - +FROM ches/kafka:0.10.2.1 Review comment: Same as above, why do we have these? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42: URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417041527 ## File path: docker/containers/kafka/Dockerfile ## @@ -16,20 +14,4 @@ # See the License for the specific language governing permissions and # limitations under the License. # Review comment: Why do we have these files? we don't need them for compose 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42: URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417041583 ## File path: docker/containers/kafka/Dockerfile ## @@ -16,20 +14,4 @@ # See the License for the specific language governing permissions and # limitations under the License. # - -shopt -s nocasematch - -# -# Waits until kafka is available on the default port -# - -cd /root || exit 1 - -if [ ! -d /root/scripts ]; then - echo "DOCKER_SCRIPTS_PATH has not been set and mapped" - exit 1 -fi - -cd /root/scripts || exit 1 -bash ./wait-for-it.sh kafka:9092 - +FROM ches/kafka:0.10.2.1 Review comment: Same as above, why do we have these? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
ottobackwards commented on a change in pull request #42: URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417041000 ## File path: docker/README.md ## @@ -209,33 +140,21 @@ testing scripts to be added to a pull request, and subsequently to a test suite. ```bash --log-directory[REQUIRED] The directory with the logs ``` -- `stop_container.sh`: Stops and removes a Docker container with a given name - ## Parameters - ```bash - --container-name [REQUIRED] The Docker container name - ``` The example end to end test script `run_end_to_end.sh` is provided as an example of a testing script. Specific or extended scripts can be created similar to this script to use the containers. This script does the following: Review comment: should these all be #1? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
