Fix facebook marketing source SAT fail on invalid config file #5621

avida · 2021-08-25T14:44:30Z

What

Resolves #5592

How

Describe the solution

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions
Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

yevhenii-ldv · 2021-08-26T13:22:15Z

.pre-commit-config.yaml

@@ -6,13 +6,13 @@ repos:
        args: ["--tmpl=LICENSE", "--ext=py", "-f"]

  - repo: https://github.com/ambv/black
-    rev: stable
+    rev: 20.8b1


Why did you decide to specify the correct version and not leave the stable?

This version are used in our gradle config.

Zirochkaa

lgtm, except 1 typo

airbyte-cdk/python/airbyte_cdk/sources/utils/schema_helpers.py

sherifnada · 2021-08-30T16:22:25Z

airbyte-cdk/python/CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog

+## 0.1.13
+In case of passed config file are not comply with spec schema raise an exception instead of sending airbyte error message and exit.


Suggested change

In case of passed config file are not comply with spec schema raise an exception instead of sending airbyte error message and exit.

If the input config file does not comply with spec schema, raise an exception instead of `system.exit`.

sherifnada · 2021-08-30T17:33:42Z

@avida why does changing from system.exit to raise excception solve the problem?

Isn't the main fix just changing failed to exception in the test config yaml?

…-fix

avida · 2021-08-31T08:35:20Z

@avida why does changing from system.exit to raise excception solve the problem?

SAT expecting something to be present on stderr in case if application exited with 1 error code, that one of the problem why SAT case failed

Isn't the main fix just changing failed to exception in the test config yaml?

Yes but also there should be a traceback on stderr which is expected by SAT.

avida · 2021-08-31T08:38:20Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/1185405413
✅ https://github.com/airbytehq/airbyte/actions/runs/1185405413

avida requested review from htrueman, Zirochkaa and yevhenii-ldv August 25, 2021 14:44

github-actions bot added area/connectors Connector related issues CDK Connector Development Kit labels Aug 25, 2021

avida force-pushed the drezchykov/5592-SAT-fix branch from 58ee0b9 to d23205d Compare August 25, 2021 14:53

Fix facebook marketing source SAT fail on invalid config file

ce6ba03

avida force-pushed the drezchykov/5592-SAT-fix branch from d23205d to ce6ba03 Compare August 25, 2021 15:17

Merge github.com:airbytehq/airbyte into drezchykov/5592-SAT-fix

f712c38

htrueman approved these changes Aug 26, 2021

View reviewed changes

avida requested a review from sherifnada August 26, 2021 10:09

yevhenii-ldv reviewed Aug 26, 2021

View reviewed changes

yevhenii-ldv approved these changes Aug 26, 2021

View reviewed changes

Zirochkaa approved these changes Aug 26, 2021

View reviewed changes

airbyte-cdk/python/airbyte_cdk/sources/utils/schema_helpers.py Outdated Show resolved Hide resolved

fix review comment

651f54e

sherifnada approved these changes Aug 30, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into drezchykov/5592-SAT…

eb35029

…-fix

avida force-pushed the drezchykov/5592-SAT-fix branch from 9041bac to eb35029 Compare August 31, 2021 08:32

avida merged commit 3aabd92 into master Aug 31, 2021

avida deleted the drezchykov/5592-SAT-fix branch August 31, 2021 08:48

jrhizor mentioned this pull request Sep 3, 2021

Bump Airbyte version from 0.29.13-alpha to 0.29.14-alpha #5831

Merged

karinakuz added connectors/sources-api connectors/source/facebook-marketing labels Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix facebook marketing source SAT fail on invalid config file #5621

Fix facebook marketing source SAT fail on invalid config file #5621

avida commented Aug 25, 2021

yevhenii-ldv Aug 26, 2021

avida Aug 27, 2021

Zirochkaa left a comment

sherifnada Aug 30, 2021

sherifnada commented Aug 30, 2021

avida commented Aug 31, 2021

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading

	In case of passed config file are not comply with spec schema raise an exception instead of sending airbyte error message and exit.
	If the input config file does not comply with spec schema, raise an exception instead of `system.exit`.

Fix facebook marketing source SAT fail on invalid config file #5621

Fix facebook marketing source SAT fail on invalid config file #5621

Conversation

avida commented Aug 25, 2021

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

yevhenii-ldv Aug 26, 2021

Choose a reason for hiding this comment

avida Aug 27, 2021

Choose a reason for hiding this comment

Zirochkaa left a comment

Choose a reason for hiding this comment

sherifnada Aug 30, 2021

Choose a reason for hiding this comment

sherifnada commented Aug 30, 2021

avida commented Aug 31, 2021

avida commented Aug 31, 2021 • edited by github-actions bot Loading

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading