-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Harmonize /test and /publish commands on github + Fix MySQL tests in CI #2531
Conversation
/test connector=connectors/source-mysql
|
ac97948
to
3a9daa2
Compare
/test connector=source-mysql
|
/test connector=bases/base-normalization
|
… MySQL container as root
/test connector=connectors/source-mysql
|
could you update the docs at https://docs.airbyte.io/contributing-to-airbyte/building-new-connector/testing-connectors? |
/test connector=source-mysql ref=master
|
@@ -11,8 +13,14 @@ if [[ "$connector" == "all" ]] ; then | |||
echo "Running: ./gradlew --no-daemon --scan integrationTest" | |||
./gradlew --no-daemon --scan integrationTest | |||
else | |||
selected_integration_test=$(echo "$all_integration_tests" | grep "^$connector$" || echo "") | |||
integrationTestCommand=":airbyte-integrations:connectors:$connector:integrationTest" | |||
if [[ "$connector" == *"connectors"* ]] || [[ "$connector" == *"bases"* ]]; then |
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.
why do we need this conditional? can't we just say you should always do it one way?
if i'm interpreting right it seems like you can do: connectors/source-postgres
or source-postgres
? i think it's the else
that feels a little iffy, because i think it'll work for source-postgres
but it won't work for base-normalization
since :connectors:
is hard coded.
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.
This condition makes it so that it keeps both the old behavior and the new one.
You are right, the old behavior (which is source-postgres
) won't work with base-normalization
as you pointed out
However, I don't understand yet why the new behavior gets rejected when used with the ref=XXX
arguments:
#2531 (comment)
but the old behavior does pass validation: #2531 (comment)
So since the new behavior doesn't fully replace the old one yet in terms of functionalities, I am leaving both options available...
/test connector=connectors/source-mysql ref=master
|
I'm late to this; but this is very cool! |
What
When we want to publish docker images, we can comment on github with
But if we want to run integration tests, it's:
How
This PR makes it so that we can also run integration tests with:
so it is easier to switch between a publish or a test comment
It is also now possible to run:
whereas, the following command will fail:
with:
This PR also fix compilation error of normalization image and tests for the MySQL sources when launched from CI