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.

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

doc: Document our connectors QA checks #35324

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion airbyte-ci/connectors/connectors_qa/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ This package uses two local dependencies:

To add a new QA check, you have to create add new class in one of the `checks` module. This class must inherit from `models.Check` and implement the `_run` method. Then, you need to add an instance of this class to the `ENABLED_CHECKS` list of the module.

**Please run the `generate-doumentation` command to update the documentation with the new check and commit it in your PR.**
**Please run the `generate-documentation` command to update the documentation with the new check and commit it in your PR.**:
```bash
# From airbyte repo root
connectors-qa generate-documentation docs/contributing-to-airbyte/resources/qa-checks.md
```

### Running tests

Expand Down
128 changes: 73 additions & 55 deletions airbyte-ci/connectors/connectors_qa/poetry.lock

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions airbyte-ci/connectors/connectors_qa/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ pytest = "^8.0.0"
pytest-mock = "^3.12.0"
mypy = "^1.8.0"
types-toml = "^0.10.8.7"
pytest-asyncio = "^0.23.5"
gitpython = "^3.1.42"

[build-system]
requires = ["poetry-core"]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class AssetsCheck(Check):

class CheckConnectorIconIsAvailable(AssetsCheck):
name = "Connectors must have an icon"
description = "Each connector must have an icon available in at the root of the connector code directory. It must be an SVG file named `icon.svg`."
description = "Each connector must have an icon available in at the root of the connector code directory. It must be an SVG file named `icon.svg` and must be a square."
requires_metadata = False

def _run(self, connector: Connector) -> CheckResult:
Expand All @@ -27,6 +27,7 @@ def _run(self, connector: Connector) -> CheckResult:
passed=False,
message="Icon file is not named 'icon.svg'",
)
# TODO check that the icon is a square
return self.create_check_result(connector=connector, passed=True, message="Icon file exists")


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class DocumentationCheck(Check):

class CheckMigrationGuide(DocumentationCheck):
name = "Breaking changes must be accompanied by a migration guide"
description = "When a breaking change is introduced we check that a migration guide is available. It should be stored under `./docs/integrations/<connector-type>s/<connector-name>-migrations.md`.\nThis document should contain a section for each breaking change, in order of the version descending. It must explain users which action to take to migrate to the new version."
description = "When a breaking change is introduced, we check that a migration guide is available. It should be stored under `./docs/integrations/<connector-type>s/<connector-name>-migrations.md`.\nThis document should contain a section for each breaking change, in order of the version descending. It must explain users which action to take to migrate to the new version."

def _run(self, connector: Connector) -> CheckResult:
breaking_changes = get(connector.metadata, "releases.breakingChanges")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def _run(self, connector: Connector) -> CheckResult:

class CheckConnectorLicenseMatchInPyproject(PackagingCheck):
name = f"Connector license in {consts.METADATA_FILE_NAME} and {consts.PYPROJECT_FILE_NAME} file must match"
description = f"Connectors license in {consts.METADATA_FILE_NAME} and {consts.PYPROJECT_FILE_NAME} file must match. This is to ensure that all connectors are consistently licensed"
description = f"Connectors license in {consts.METADATA_FILE_NAME} and {consts.PYPROJECT_FILE_NAME} file must match. This is to ensure that all connectors are consistently licensed."
applies_to_connector_languages = [
ConnectorLanguage.PYTHON,
ConnectorLanguage.LOW_CODE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ They are by no mean replacing the need for a manual review of the connector code
*Applies to the following connector languages: {{ ', '.join(check.applies_to_connector_languages) }}*

{{ check.description }}
{% endfor %}
{%- endfor %}
{% endfor %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Copyright (c) 2023 Airbyte, Inc., all rights reserved.


from pathlib import Path

import git
import pytest
from asyncclick.testing import CliRunner
from connectors_qa.cli import generate_documentation

DOCUMENTATION_FILE_PATH_IN_AIRBYTE_REPO = Path("docs/contributing-to-airbyte/resources/qa-checks.md")

@pytest.fixture
def airbyte_repo():
return git.Repo(search_parent_directories=True)

@pytest.mark.asyncio
async def test_generated_qa_checks_documentation_is_up_to_date(airbyte_repo, tmp_path):
# Arrange
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this organization of test. I would love to see this reflected in the test naming too. I usually use "given_x_when_y_then_z". I hate Python snake case because it feels like it would be easier to read having something like givenX_whenY_thenZ but eh!

current_doc = (airbyte_repo.working_dir / DOCUMENTATION_FILE_PATH_IN_AIRBYTE_REPO).read_text()
newly_generated_doc_path = tmp_path / "qa-checks.md"

# Act
await CliRunner().invoke(generate_documentation, [str(tmp_path / "qa-checks.md")], catch_exceptions=False)

# Assert
suggested_command = f"connectors-qa generate-documentation {DOCUMENTATION_FILE_PATH_IN_AIRBYTE_REPO}"
assert newly_generated_doc_path.read_text() == current_doc, f"The generated documentation is not up to date. Please run `{suggested_command}` and commit the changes."
4 changes: 2 additions & 2 deletions docs/contributing-to-airbyte/change-cdk-connector.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ Make sure you're working on an issue had been already triaged to not have your c
3. Code the change
4. Write a unit test for each custom function you added or changed
5. Ensure all tests, including connector acceptance tests, pass
6. Update the `metadata.yaml` and `Dockerfile` version following the [guidelines](./resources/pull-requests-handbook.md#semantic-versioning-for-connectors)
6. Update the `metadata.yaml` following the [guidelines](./resources/pull-requests-handbook.md#semantic-versioning-for-connectors)
7. Update the changelog entry in documentation in `docs/integrations/<connector-name>.md`
8. Make sure your contribution passes our [QA checks](./resources/qa-checks.md)

A comment will automatically be added to your PR with a checklist containing the necessary steps to complete your contribution and get it merged.

:::info
There is a README file inside each connector folder containing instructions to run that connector's tests locally.
Expand Down
81 changes: 81 additions & 0 deletions docs/contributing-to-airbyte/resources/qa-checks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# Airbyte connectors QA checks

This document is listing all the static-analysis checks that are performed on the Airbyte connectors.
These checks are running in our CI/CD pipeline and are used to ensure a connector is following the best practices and is respecting the Airbyte standards.
Meeting these standards means that the connector will be able to be safely integrated into the Airbyte platform and released to registries (DockerHub, Pypi etc.).
You can consider these checks as a set of guidelines to follow when developing a connector.
They are by no mean replacing the need for a manual review of the connector codebase and the implementation of good test suites.


## 📄 Documentation

### Breaking changes must be accompanied by a migration guide
*Applies to the following connector languages: java, low-code, python*

When a breaking change is introduced, we check that a migration guide is available. It should be stored under `./docs/integrations/<connector-type>s/<connector-name>-migrations.md`.
This document should contain a section for each breaking change, in order of the version descending. It must explain users which action to take to migrate to the new version.
### Connectors must have user facing documentation
*Applies to the following connector languages: java, low-code, python*

The user facing connector documentation should be stored under `./docs/integrations/<connector-type>s/<connector-name>.md`.
### Connectors documentation follows our guidelines
*Applies to the following connector languages: java, low-code, python*

The user facing connector documentation should follow the guidelines defined in the [documentation standards](https://hackmd.io/Bz75cgATSbm7DjrAqgl4rw).
### Connectors must have a changelog entry for each version
*Applies to the following connector languages: java, low-code, python*

Each new version of a connector must have a changelog entry defined in the user facing documentation in `./docs/integrations/<connector-type>s/<connector-name>.md`.

## 📝 Metadata

### Connectors must have valid metadata.yaml file
*Applies to the following connector languages: java, low-code, python*

Connectors must have a `metadata.yaml` file at the root of their directory. This file is used to build our connector registry. Its structure must follow our metadata schema. Field values are also validated. This is to ensure that all connectors have the required metadata fields and that the metadata is valid. More details in this [documentation](https://docs.airbyte.com/connector-development/connector-metadata-file).

## 📦 Packaging

### Connectors must use Poetry for dependency management
*Applies to the following connector languages: python, low-code*

Connectors must use [Poetry](https://python-poetry.org/) for dependency management. This is to ensure that all connectors use a dependency management tool which locks dependencies and ensures reproducible installs.
### Connectors must be licensed under MIT or Elv2
*Applies to the following connector languages: java, low-code, python*

Connectors must be licensed under the MIT or Elv2 license. This is to ensure that all connectors are licensed under a permissive license. More details in our [License FAQ](https://docs.airbyte.com/developer-guides/licenses/license-faq).
### Connector license in metadata.yaml and pyproject.toml file must match
*Applies to the following connector languages: python, low-code*

Connectors license in metadata.yaml and pyproject.toml file must match. This is to ensure that all connectors are consistently licensed.
### Connector version must follow Semantic Versioning
*Applies to the following connector languages: java, low-code, python*

Connector version must follow the Semantic Versioning scheme. This is to ensure that all connectors follow a consistent versioning scheme. Refer to our [Semantic Versioning for Connectors](https://docs.airbyte.com/contributing-to-airbyte/#semantic-versioning-for-connectors) for more details.
### Connector version in metadata.yaml and pyproject.toml file must match
*Applies to the following connector languages: python, low-code*

Connector version in metadata.yaml and pyproject.toml file must match. This is to ensure that connector release is consistent.
### Python connectors must have PyPi publishing enabled
*Applies to the following connector languages: python, low-code*

Python connectors must have [PyPi](https://pypi.org/) publishing enabled in their `metadata.yaml` file. This is declared by setting `remoteRegistries.pypi.enabled` to `true` in metadata.yaml. This is to ensure that all connectors can be published to PyPi and can be used in `airbyte-lib`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: If this is a must, why don't we just remove remoteRegistries.pypi.enabled and release to pypi when it is python/low-code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I like the explicitness of the metadata flag and the possibility to disable publishing easily if we ever need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to me. Could we solve this by having a default value but still having the flag so that we can explicitly disable it if necessary? I think this is very low priority though


## 💼 Assets

### Connectors must have an icon
*Applies to the following connector languages: java, low-code, python*

Each connector must have an icon available in at the root of the connector code directory. It must be an SVG file named `icon.svg` and must be a square.

## 🔒 Security

### Connectors must use HTTPS only
*Applies to the following connector languages: java, low-code, python*

Connectors must use HTTPS only when making requests to external services.
### Python connectors must not use a Dockerfile and must declare their base image in metadata.yaml file
*Applies to the following connector languages: python, low-code*

Connectors must use our Python connector base image (`docker.io/airbyte/python-connector-base`), declared through the `connectorBuildOptions.baseImage` in their `metadata.yaml`.
This is to ensure that all connectors use a base image which is maintained and has security updates.
1 change: 1 addition & 0 deletions docs/contributing-to-airbyte/submit-new-connector.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ This will enable our team to make sure your contribution does not overlap with e
3. Code the change
4. Ensure all tests pass. For connectors, this includes acceptance tests as well.
5. Update documentation in `docs/integrations/<connector-name>.md`
6. Make sure your contribution passes our [QA checks](./resources/qa-checks.md)


#### Open a pull request
Expand Down
1 change: 1 addition & 0 deletions docusaurus/sidebars.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ const contributeToAirbyte = {
"contributing-to-airbyte/resources/pull-requests-handbook",
"contributing-to-airbyte/resources/code-style",
"contributing-to-airbyte/resources/code-formatting",
"contributing-to-airbyte/resources/qa-checks",
"contributing-to-airbyte/resources/developing-locally",
"contributing-to-airbyte/resources/developing-on-docker",
],
Expand Down
Loading