-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove release stage references in favor of support level or ab_internal #29232
Conversation
…ert/update-ci-check
BACKWARD_COMPATIBILITY_REVIEWERS = {"connector-operations", "connector-extensibility"} | ||
TEST_STRICTNESS_LEVEL_REVIEWERS = {"connector-operations"} | ||
GA_BYPASS_REASON_REVIEWERS = {"connector-operations"} | ||
GA_CONNECTOR_REVIEWERS = {"gl-python"} | ||
REVIEW_REQUIREMENTS_FILE_PATH = ".github/connector_org_review_requirements.yaml" | ||
|
||
def is_important_connector(connector: utils.Connector) -> bool: |
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.
Opted to use the idea of important connector which is either a high sl and/or high ql.
This is because we may have a connector that is deemed qualitatively important (SL) and we should enable high strictness in advance of a high QL.
defn: https://docs.google.com/document/d/161FF__wOjwS6j-1fVyHexDD1cJCQ5RJ2LMC8FI2N6yI/edit
The alternative is to use just QL.
"ql": 300, | ||
} | ||
|
||
def _requires_allowed_hosts(connector: utils.Connector) -> bool: |
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.
return sl_value | ||
|
||
@property | ||
def ab_internal_ql(self) -> int: |
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.
These should already be defined but putting defaults here in case.
These should be converted to hard errors on the introduction of metadata v2 where ab_internal
is required
source-s3 test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-s3/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-s3 docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Integration tests | ❌ |
Acceptance tests | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-s3 test
def ab_internal_sl(self) -> int: | ||
"""Airbyte Internal Field. | ||
|
||
More info can be found here: https://www.notion.so/Internal-Metadata-Fields-32b02037e7b244b7934214019d0b7cc9 |
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.
Added a new page to notion since our wiki is now deprecated
This reverts commit 1861b95.
source-s3 test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-s3/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-s3 docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Integration tests | ✅ |
Acceptance tests | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-s3 test
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.
Kudos for tackling this!
Nit comments. LGTM as long as all the tests are 🟢 .
I believe the next step is to remove the releaseStage from the metadata model + update all metadata.yaml files?
@@ -11,14 +11,14 @@ on: | |||
default: conn-nightly-xlarge-runner | |||
required: true | |||
test-connectors-options: | |||
default: --concurrency=5 --release-stage=generally_available --release-stage=beta | |||
default: --concurrency=5 --support-level=certified |
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.
All the metadata are up to date to match the same subset of connector nightly build originally targets?
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.
They dont match the same subset unfortunately. There will likely be a bit of a mix up
BACKWARD_COMPATIBILITY_REVIEWERS = {"connector-operations", "connector-extensibility"} | ||
TEST_STRICTNESS_LEVEL_REVIEWERS = {"connector-operations"} | ||
GA_BYPASS_REASON_REVIEWERS = {"connector-operations"} | ||
GA_CONNECTOR_REVIEWERS = {"gl-python"} | ||
REVIEW_REQUIREMENTS_FILE_PATH = ".github/connector_org_review_requirements.yaml" | ||
|
||
def is_important_connector(connector: utils.Connector) -> bool: |
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.
Would love to see this exposed as a Connector property as it might be useful in pipelines
too.
requires_high_test_strictness_level = is_important_connector(connector) | ||
check_for_high_strictness = connector.acceptance_test_config is not None and requires_high_test_strictness_level |
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.
Wondering if requires_high_test_strictness_level
could be a Connector property too.
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.
absolutely!
@@ -100,79 +100,6 @@ def compute_registry_overrides(merged_df): | |||
return registries | |||
|
|||
|
|||
def merge_into_metadata_definitions( |
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 it going away?
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 was an artifact from when we transformed the old registry into the metadata files at the very begining.
Because we no longer do that, this function is unused, it just happened to be missed by the grim reaper last time
help="Filter connectors to test by release stage.", | ||
type=click.Choice(["alpha", "beta", "generally_available"]), | ||
help="Filter connectors to test by support_level.", | ||
type=click.Choice(["community", "certified"]), |
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: I did not made the extra mile to create an enum for releases stages, but I think it's worth it. (doing the same as ConnectorLanguage)
@@ -14,12 +14,10 @@ class ConnectorTypeEnum(str, Enum): | |||
source = "source" | |||
destination = "destination" | |||
|
|||
|
|||
class ReleaseStageEnum(str, Enum): | |||
class SupportLevelEnum(str, Enum): |
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.
Ahah the enum exists here :) Might be worth migrating it to connector_ops.utils...
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.
Bump the pipelines
version in pyproject.toml and add a changelog entry please 😄
source-amazon-ads test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-amazon-ads/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ❌ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-amazon-ads docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-amazon-ads test
This reverts commit dce9b8e.
…nal (airbytehq#29232) * First pass on removing release stage references * Automated Commit - Format and Process Resources Changes * Fix metadata tests * Fix qa engine tests * Fix pipeline tests * Update registry report * Fix support-level arg * Add dummy change * Deal with --allow_community * Improve log message * Add docs * Revert "Add dummy change" This reverts commit 1861b95. * Code review comments * Run format * Install connector ops to qa-engine * Fix CI test * REVERT ME * Revert "REVERT ME" This reverts commit dce9b8e. --------- Co-authored-by: bnchrch <bnchrch@users.noreply.github.com>
Overview
Updates the following to use the new support levels and ab_internal fields
closes #28713 and #28710