-
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
✨ Source Sentry: migrate to low code #35755
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
releases: | ||
breakingChanges: | ||
1.0.0: | ||
message: "Source Sentry has been migrated to YAML Declarative Source." |
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 is this a breaking change?
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.
+1. Seems like schemas have not changed?
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.
@girarda @natikgadzhi yes, actually this is not breaking change and low-code version of the connector is compatible with the previous one. According to the migration to low-code guide I thought we migrate all connectors to low-code as breaking change. If it's not I can remove breaking change docs from the pr.
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.
If the config, the state, and the schema are compatible, we shouldn't mark this as a breaking change. CC @katmarkham
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.
Thank you @darynaishchenko! 🎉
My only question is the same as from @girarda — not sure if the schema changes mandate this to be a breaking change.
@clnoll, should we try and use the regression testing tool on this?
@@ -1,3 +1,3 @@ | |||
[run] | |||
omit = | |||
omit = | |||
source_sentry/run.py |
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.
Some folks had to also add main.py
and __init__.py
, but if the coverage is hit, I guess we're okay /shrug
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.
Note to self: this file should be in the template for new connectors
@@ -4,7 +4,7 @@ acceptance_tests: | |||
- config_path: secrets/config.json | |||
empty_streams: | |||
- name: issues | |||
bypass_reason: "Project sssues are not being returned by the Sentry API." | |||
bypass_reason: "Project issues are not being returned by the Sentry API." |
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.
Thank you for catching these 👏
@@ -1,105 +1,149 @@ | |||
version: "0.29.0" | |||
version: 0.57.0 |
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'm a bit confused by this, and I see this in a couple PRs (another one is #35776). Should manifest.version
be the minimal version of the CDK that can run this, or should it be the version of the connector? If it's the connector version, should this be 1.0.0?
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 version was added by the connector builder, this is version of cdk, I'm confused with this too, because poetry lock declares another version of cdk. So it looks like I need to update version in manifest.
page_token_option: | ||
type: RequestOption | ||
inject_into: "request_parameter" | ||
field_name: "cursor" | ||
inject_into: request_parameter |
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.
Out of curiosity: what formatting tool are you using? I want to make sure we will setup our tooling so formatting is consistent across the team
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 used manual + IDE + airbyte-ci formatting. page_token_option:
was added and formatted by connector builder.
Projects(**stream_args), | ||
Releases(**project_stream_args), | ||
] | ||
class SourceSentry(YamlDeclarativeSource): |
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.
Nice that all streams could move to declarative. 👏
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.
Yes, one thing that is different in new and old version is state calculation. Now states equals date of sync, previously it was date of recent record. But for this streams it can be used as dateCreated
and lastSeen
values can not be updated manually, so date of cursor can not be changed after sync.
releases: | ||
breakingChanges: | ||
1.0.0: | ||
message: "Source Sentry has been migrated to YAML Declarative Source." |
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.
+1. Seems like schemas have not changed?
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.
Two concerns i'd like to address before approving (the rest is nit):
- the API call change for issues
- the semi-incremental thing for
releases
andprojects
airbyte-integrations/connectors/source-sentry/source_sentry/manifest.yaml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-sentry/source_sentry/manifest.yaml
Show resolved
Hide resolved
airbyte-integrations/connectors/source-sentry/source_sentry/manifest.yaml
Show resolved
Hide resolved
airbyte-integrations/connectors/source-sentry/source_sentry/manifest.yaml
Show resolved
Hide resolved
airbyte-integrations/connectors/source-sentry/source_sentry/manifest.yaml
Show resolved
Hide resolved
airbyte-integrations/connectors/source-sentry/source_sentry/manifest.yaml
Show resolved
Hide resolved
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.
Awesome work @darynaishchenko ! 🚢
|
||
@HttpMocker() | ||
def test_read(self, http_mocker: HttpMocker): | ||
response = [ |
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.
nit: please move responses to unit_tests/resource/http/response
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.
updated
What
resolved: https://github.com/airbytehq/airbyte-internal-issues/issues/6632
Migrate Source Sentry to Low Code.
How
Updated
manifest.yaml
with declared in low code streams. Added integration tests for empty streamsIssues
andEvents
.Recommended reading order
airbyte-integrations/connectors/source-sentry/source_sentry/manifest.yaml
🚨 User Impact 🚨
No breaking changes.
Sentry low code migration qa