Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🎉 Source Github: bugfix schemas for streams deployments, workflow_runs, teams #15049

Merged
merged 9 commits into from
Jul 29, 2022
Merged

🎉 Source Github: bugfix schemas for streams deployments, workflow_runs, teams #15049

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
35f3d51
improve schemas
grubberr Jul 26, 2022
6c949cb
github.md updated
grubberr Jul 26, 2022
8dc57a8
revert TypeTransformer
grubberr Jul 26, 2022
500fa97
Merge branch 'master' into grubberr/oncall-321-2-source-github
grubberr Jul 27, 2022
dbd6b2b
updated deployments schema added oneOf
grubberr Jul 27, 2022
68c48fc
improve "additionalProperties": true
grubberr Jul 27, 2022
afba045
Merge branch 'master' into grubberr/oncall-321-2-source-github
grubberr Jul 27, 2022
c3e0abe
Merge branch 'master' into grubberr/oncall-321-2-source-github
grubberr Jul 29, 2022
f480c87
auto-bump connector version [ci skip]
octavia-squidington-iii Jul 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion airbyte-integrations/connectors/source-github/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ RUN pip install .
ENV AIRBYTE_ENTRYPOINT "python /airbyte/integration_code/main.py"
ENTRYPOINT ["python", "/airbyte/integration_code/main.py"]

LABEL io.airbyte.version=0.2.42
LABEL io.airbyte.version=0.2.43
LABEL io.airbyte.name=airbyte/source-github
14 changes: 13 additions & 1 deletion airbyte-integrations/connectors/source-github/source_github/schemas/deployments.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,19 @@
"type": ["null", "string"]
},
"payload": {
"type": ["null", "string", "object"]
"oneOf": [
{
"type": "object",
"properties": {},
"additionalProperties": true
},
{
"type": "string"
},
{
"type": "null"
}
]
Comment on lines +106 to +118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://github.com/airbytehq/airbyte/issues/15021, oneOf, anyOf, and not should not be allowed in SAT

@sherifnada what's the actual effect of doing this?

This should probably just be a string type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I suspected this question
The problem is if remove this to multi-type, normalization will start to fail (at least destination-snowflake)

I need your approval to remove this multi-type !!!

The second point why I have re-implemented using oneOf instead of just simple ["string", "object"] because it's better handled by python Transformer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other words you can see here just refactoring of previous schema
but making it more compatible with python Transformer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grubberr whats the failure in normalization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like this
https://github.com/airbytehq/airbyte/issues/15021#issuecomment-1196320638

In short, all multi-type(s) combinations usually stored as pure-JSON on destination
on some engine it's VARCHAR but it's not always

for example:

on snowflake - VARIANT type
on redshift - SUPER type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Got it - sending a string would actually cause an error since they're currently already being handled as objects. Then in this case to avoid the breaking schema change, since payload is an arbitrary object I think it's ok to make our own object with the string value.

If we confirm that normalization/destinations are just handling this as an object, then we could update the schema here to be ["null", "object"] (remove string). @grubberr you mentioned earlier that for validation this wouldn't be ok - what issues do we have with validation if we do this?

In practice this should only be:

object -> object
"string" -> {"value": "string"}
null -> null

(if github docs are true to their word)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroslopez if we do such transformation
"string" -> {"value": "string"}
no issues with validation

Copy link
Contributor

@pedroslopez pedroslopez Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked earlier whether we were seeing an issue with this currently since, while we should fix these multi-type schemas, it seems like a separate issue from what's being addressed in this PR for the on call issue. Since it's on call, I think we should move forward with the current changes since it's behavior is equivaltent and address the schema issue / coercing strings into objects separately

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh github keeps hiding previous comments before I post 😛
@grubberr if you have those changes made then feel free to include here or separately, up to you

Copy link
Contributor Author

@grubberr grubberr Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroslopez no I did not do such changes
"string" -> {"value": "string"}
it's was only my discussion about possible solutions how to move forward to be compliant with this PR https://github.com/airbytehq/airbyte/issues/15021

},
"transient_environment": {
"type": ["null", "boolean"]
Expand Down
4 changes: 3 additions & 1 deletion airbyte-integrations/connectors/source-github/source_github/schemas/teams.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@
"type": ["null", "string"]
},
"parent": {
"type": ["null", "object"]
"type": ["null", "object"],
"properties": {},
"additionalProperties": true
}
}
}
6 changes: 5 additions & 1 deletion airbyte-integrations/connectors/source-github/source_github/schemas/workflow_runs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@
},
"pull_requests": {
"type": "array",
"items": {}
"items": {
"type": ["null", "object"],
"properties": {},
"additionalProperties": true
}
},
"created_at": {
"type": ["null", "string"]
Expand Down
1 change: 1 addition & 0 deletions docs/integrations/sources/github.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ The GitHub connector should not run into GitHub API limitations under normal usa

| Version | Date | Pull Request | Subject |
|:--------|:-----------| :--- |:-------------------------------------------------------------------------------------------------------------|
| 0.2.43 | 2022-07-26 | [15049](https://github.com/airbytehq/airbyte/pull/15049) | Bugfix schemas for streams `deployments`, `workflow_runs`, `teams` |
| 0.2.42 | 2022-07-12 | [14613](https://github.com/airbytehq/airbyte/pull/14613) | Improve schema for stream `pull_request_commits` added "null" |
| 0.2.41 | 2022-07-03 | [14376](https://github.com/airbytehq/airbyte/pull/14376) | Add Retry for GraphQL API Resource limitations |
| 0.2.40 | 2022-07-01 | [14338](https://github.com/airbytehq/airbyte/pull/14338) | Revert: "Rename field `mergeable` to `is_mergeable`" |
Expand Down