Connector Levels: Add new internal metadata fields #28904

bnchrch · 2023-08-01T08:44:35Z

Overview

closes #28712

Adds new internal fields to registry and metadata types
Populates them according to https://docs.google.com/spreadsheets/d/17tuDHfCJwwyGF6DyrwI3oZKM2UIhvHsyhYFRKTKrWFU/edit#gid=768783834
Fills in the remaining by assuming the destination and sources have the same level

github-actions · 2023-08-01T08:44:56Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

github-actions · 2023-08-01T09:21:00Z

Coverage report for source-postgres

There is no coverage information present for the Files changed

Total Project Coverage	70.27%	🍏

erohmensing

Looks reasonable! Only question is about required/defaults or not

erohmensing · 2023-08-01T17:36:58Z

...tors/metadata_service/lib/metadata_service/models/generated/ConnectorMetadataDefinitionV0.py

+class ReleaseStage1(BaseModel):
+    __root__: Literal["community", "certified"] = Field(
+        ...,
+        description="enum that describes a connector's release stage",
+        title="ReleaseStage",


is it still called ReleaseStage in the new setup? I thought they were "quality level"s

This is wrong! fixing

erohmensing · 2023-08-01T17:38:32Z

...data_service/lib/metadata_service/models/generated/ConnectorRegistryDestinationDefinition.py

@@ -154,6 +170,7 @@ class Config:
        False, description="whether this is a custom connector definition"
    )
    releaseStage: Optional[ReleaseStage] = None
+    supportLevel: Optional[ReleaseStage1] = None


Ah, this explains my question earlier - though I did find this confusing

Ah missed this! will fix up!

erohmensing · 2023-08-01T17:40:16Z

airbyte-ci/connectors/metadata_service/lib/metadata_service/models/src/AirbyteInternal.yaml

+description: Fields for internal use only
+type: object
+additionalProperties: false
+properties:


are these properties required? (I think it makes sense for these to be required, and to default to the lowest values if not provided (i.e. a new connector)

if so we should be able to apply the required and the defaults here (I think, on the defaults at least)

If they are required, we should add one to the invalid examples where the properties don't exist.

I have the same overall question for supportLevel - is it a required property?

Originally I had left it as not a required property so as not to break metadata processing at this time.

Once these are out and the systems are updated the plan is to cut a v2 of the metadata spec

Deprecate releaseStage in Metadata Schema

I'm not sure how adding this as required would break metadata processing since you've manually patched all of the files so far. Can you elaborate?

There are still old metadata files that can be processed.

For example at this time if you were to rerun the add metadata partition job in our orchestrator we would see a lot of failures for previous versions of metadata files.

This isnt a major issue, and overtime we expect there are old metadata files we cant/no longer want to process. Hell even right now we fail silently as long as this doesnt occur on a latest file path.

However I believe its good practise that while were still updating our downstream systems to use these new fields we allow them to exist harmoniously with the existing (old) systems. That way we can always rollback changes to downstream systems in case things go wrong.

Also it delays me from cutting the v2 of the metadata spec. And the more we delay their the more we learn. and the more we learn the less likely we end up in a situation where we have to cut a v3 immediately afterward.

Great idea! Also Ill add the defaults to the scaffold, but I want them to set them explicitly! If we add defaults here then they never have to add them to the metadata

mm yeah i'm mostly concerned about community contributors - though I guess they'd not add the ab_internal stuff at all

in that case no defaults but still reqired sub-keys makes sense to me!

Fair call out. These are akward that way.

I think this means that the defaults for this fields are pushed to the consuming service.

In this case the transformation from metadata to registry entry

Meaning for the Registry we can make these required and fill in the defaults when transforming from the metadata definition

bnchrch · 2023-08-01T23:08:00Z

/approve-and-merge reason="Adding benign metadata fields"

octavia-approvington · 2023-08-01T23:08:32Z

A crack team of mammals has made a decision.

* Add airbyte internal * Add tests * First pass * Set destinations to same levels as sources * Best guess at missing statuses * Best guess at _ql * Add separate enum class * Fix support level name * Update templates * Add one more test

bnchrch requested review from a team as code owners August 1, 2023 08:44

octavia-squidington-iii added the area/connectors Connector related issues label Aug 1, 2023

bnchrch requested a review from a team August 1, 2023 15:41

erohmensing approved these changes Aug 1, 2023

View reviewed changes

bnchrch added 10 commits August 1, 2023 15:16

Add airbyte internal

6b46b8a

Add tests

ff5ee0e

First pass

5af8f7a

Set destinations to same levels as sources

42ac3ec

Best guess at missing statuses

1974bf7

Best guess at _ql

032b33d

Add separate enum class

eb1c643

Fix support level name

99c46fb

Update templates

9218f4d

Add one more test

3dc8b34

bnchrch force-pushed the bnchrch/connector-cert/add-metadata-definition branch from 2d85dcc to 3dc8b34 Compare August 1, 2023 22:17

bnchrch requested a review from a team as a code owner August 1, 2023 22:17

bnchrch added the checklist-action-run label Aug 1, 2023

octavia-approvington approved these changes Aug 1, 2023

View reviewed changes

octavia-approvington merged commit e9490e3 into master Aug 1, 2023

octavia-approvington deleted the bnchrch/connector-cert/add-metadata-definition branch August 1, 2023 23:08

evantahler mentioned this pull request Aug 16, 2023

Only GA destinations should be certified #29499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connector Levels: Add new internal metadata fields #28904

Connector Levels: Add new internal metadata fields #28904

bnchrch commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited by bnchrch

Loading

github-actions bot commented Aug 1, 2023

erohmensing left a comment

erohmensing Aug 1, 2023

bnchrch Aug 1, 2023

erohmensing Aug 1, 2023

bnchrch Aug 1, 2023

erohmensing Aug 1, 2023

erohmensing Aug 1, 2023

bnchrch Aug 1, 2023

erohmensing Aug 1, 2023

bnchrch Aug 1, 2023

bnchrch Aug 1, 2023

erohmensing Aug 1, 2023

erohmensing Aug 1, 2023

bnchrch Aug 1, 2023

bnchrch Aug 1, 2023

bnchrch commented Aug 1, 2023

octavia-approvington commented Aug 1, 2023

Connector Levels: Add new internal metadata fields #28904

Connector Levels: Add new internal metadata fields #28904

Conversation

bnchrch commented Aug 1, 2023 • edited Loading

Overview

github-actions bot commented Aug 1, 2023 • edited by bnchrch Loading

Before Merging a Connector Pull Request

github-actions bot commented Aug 1, 2023

Coverage report for source-postgres

erohmensing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnchrch commented Aug 1, 2023

octavia-approvington commented Aug 1, 2023

bnchrch commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited by bnchrch

Loading