-
Notifications
You must be signed in to change notification settings - Fork 13
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
Tests for tcp.echo, tls.echo and http.kafka.sync examples #126
Conversation
1168753
to
c6c1b91
Compare
9f67235
to
94cf174
Compare
6bc2ad9
to
11b41e4
Compare
http.kafka.sync/test.sh
Outdated
if [ -z "$CORRELATION_ID" ]; then | ||
echo ❌ | ||
exit 1 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to repeat the failure output here when the test fails early?
In the general case, where it is possible to fail early in N different ways, this would be repeated N times.
For example, if each part is a function, then the execution invokes each function in turn, skipping over functions after early failure, falling through to output success or failure once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually my preference is to repeat those two lines (echo x; exit 1) whenever it makes sense to exit prematurely as opposed to introduce complex control flow with functions and such. This way we can keep a linear execution flow with the only exception of exiting early in case of fatal errors. I hope it's OK this way.
- name: Install kcat | ||
working-directory: /tmp | ||
run: | | ||
set -x | ||
curl -L -o kcat https://github.com/attilakreiner/kcat/releases/download/1.7.1/kcat-linux-$(arch) | ||
chmod +x kcat | ||
sudo mv kcat /usr/local/bin | ||
kcat 2>&1 | grep "Version 1.7.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use apt
package kafkacat
instead of this manual install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The apt package kafkacat
installs kafkacat version 1.6.0 that means it's lagging ca. one year behind the latest release. Even if we installed that we'd need to do some manual steps (e.g. renaming the executable from kafkacat to kcat) to keep the test in sync with the README and executable on other systems (e.g. on a Mac which has the latest kcat installed with brew).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I tested it, and that approach runs the test green, too. This is the alternative configuration:
- name: Install apt packages
uses: awalsh128/cache-apt-pkgs-action@latest
with:
packages: kafkacat
version: 1.0
- name: Setup kcat
run: |
sudo ln -s /usr/bin/kafkacat /usr/local/bin/kcat
kcat 2>&1 | grep "Version 1.6.0"
Pls LMK which one you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the apt install commit as discussed, so going back to manual installation of kcat, as the decision was to install the latest version.
c8515a3
to
002c6b8
Compare
This reverts commit fa4c1fa.
This change introduces automated tests for
tcp.echo
,tls.echo
andhttp.kafka.sync
examples.Fixes #125