You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by GitBox <gi...@apache.org> on 2020/04/29 02:46:19 UTC
[GitHub] [metron-bro-plugin-kafka] JonZeolla opened a new pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
JonZeolla opened a new pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42
## Contributor Comments
This is a first step towards `docker-compose` in our end to end tests.
### Testing
Run `./run_end_to_end.sh` and `./finish_end_to_end.sh`
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron's Bro kafka writer plugin.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [X] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [X] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [X] Have you included steps or a guide to how the change may be verified and tested manually?
- [X] Have you ensured that the full suite of tests and checks have been executed via:
```
bro-pkg test $GITHUB_USERNAME/metron-bro-plugin-kafka --version $BRANCH
```
- [ ] Have you written or updated unit tests and or integration tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Apache Metron's [Vagrant full-dev environment](https://github.com/apache/metron/tree/master/metron-deployment/development/centos6) or the equivalent?
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417050898
##########
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:
I may, let me think on it. Was hoping to remove the MIT license and all the overhead but compose isn't as capable as k8s readiness
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417050279
##########
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:
Correct, not required. I did it both ways but chose to keep this because I was looking to follow on with moving commands into the containers instead of docker exec/run scripts, maybe making a new entry point.
I actually did this work twice because the first time there was too much change and it with have been difficult to review.
I can remove for now and add later if you prefer? At the end of the day, it's just the same layers tagged differently
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417045301
##########
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:
It doesn't work that way. Same with healthcheck. If you look in the docker compose file I'm already using 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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#issuecomment-620974069
I am +1 on this. Review and run, seems good to me. The wait for it can be a follow on. Please create a jira.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417359367
##########
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:
I have a solution for this which is `docker-compose` native, just need to find some time to wrap it up.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417045301
##########
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:
It doesn't work that way. Same with healthcheck
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] ottobackwards commented on pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#issuecomment-621382174
+1 nice work
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#issuecomment-621248256
Thanks - give me a bit to wrap up responses to your feedback, hopefully I'll get time in the next couple of days.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417045301
##########
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:
It doesn't work that way.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417038403
##########
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:
@ottobackwards Open to suggestions - would prefer not need to go back to using `wait-for-it.sh` but that seems to be a rather [standard approach](https://docs.docker.com/compose/startup-order/) to solving this problem.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#issuecomment-621380805
Ok, back to you @ottobackwards should have addressed all of your feedback now
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [metron-bro-plugin-kafka] JonZeolla commented on a change in pull request #42: METRON-2347: [BRO-PLUGIN-KAFKA] Use docker compose in end to end tests
Posted by GitBox <gi...@apache.org>.
JonZeolla commented on a change in pull request #42:
URL: https://github.com/apache/metron-bro-plugin-kafka/pull/42#discussion_r417045900
##########
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:
Yes because it auto numbers in the rendered version
----------------------------------------------------------------
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org