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

feat(airbyte-ci): include components.py in manifest-only build #44879

Merged
merged 15 commits into from
Sep 3, 2024

Conversation

ChristoGrab
Copy link
Contributor

@ChristoGrab ChristoGrab commented Aug 29, 2024

What

Mounts components.py when building a manifest-only connector, as a prerequisite to enabling support for custom components in manifest-only connectors, and the connector builder.

Resolves #9527

How

Checks for an existing components.py file at the root level of the connector's directory. If it exists, the file is mounted alongside the manifest.

For existing low-code connectors, components.py resides in the python source code directory. It will be relocated to the root level of the folder alongisde the manifest when these connectors are migrated to manifest-only (Link to migration PR)

Review guide

Link to loom

User Impact

None. This is a step to enabling support for custom components in manifest-only conenctors.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Aug 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2024 9:48pm

@ChristoGrab ChristoGrab changed the title feat: include components in manifest-only build feat(airbyte-ci): include components.py in manifest-only build Aug 29, 2024
@ChristoGrab ChristoGrab marked this pull request as ready for review August 29, 2024 15:57
@ChristoGrab ChristoGrab requested a review from a team as a code owner August 29, 2024 15:57
Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

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

Just needs 1 test :)

base_container = self._get_base_container(platform).with_file(
f"source_declarative_manifest/{MANIFEST_FILE_PATH}",
(await self.context.get_connector_dir(include=[MANIFEST_FILE_PATH])).file(MANIFEST_FILE_PATH),
)

# Mount components file if it exists
components_file = self.context.connector.code_directory / COMPONENTS_FILE_PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

💎 This seems great to me.

@@ -183,8 +183,7 @@ async def test_check_path_in_workdir(dagger_client):
.with_workdir(str(connector.code_directory))
)
assert await utils.check_path_in_workdir(container, "metadata.yaml")
assert await utils.check_path_in_workdir(container, "pyproject.toml")
assert await utils.check_path_in_workdir(container, "poetry.lock")
assert await utils.check_path_in_workdir(container, "manifest.yaml")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this test as this connector is now manifest-only and does not have poetry files

Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

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

Pre-approving! But! Please look at the couple comments I made.

Theres a chance we have some changes that dont belong?

@@ -23,7 +23,7 @@ def mock_diffed_branched(mocker):

@pytest.fixture
def pokeapi_metadata_path():
return "airbyte-integrations/connectors/source-pokeapi/metadata.yaml"
return "airbyte-integrations/connectors/source-zoho-crm/metadata.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ This change doesnt seem to belong?

Copy link
Contributor Author

@ChristoGrab ChristoGrab Sep 3, 2024

Choose a reason for hiding this comment

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

Haha sorry, was typing the explanation at the same time, here 'tis:

Updated as source Pokeapi is now manifest-only, which breaks a couple test cases for expected reviewers (ie, marketplace team gets added). Zoho CRM is written in Python, which makes it unlikely to be migrated to manifest-only anytime soon.

@@ -37,7 +37,7 @@ def not_tracked_change_expected_team(tmp_path, pokeapi_metadata_path):
backup_path = tmp_path / "non_strategic_acceptance_test_config.backup"
shutil.copyfile(pokeapi_metadata_path, backup_path)
with open(pokeapi_metadata_path, "a") as metadata_file:
metadata_file.write("not_tracked")
metadata_file.write("\nnot_tracked: true\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ This change doesnt seem to belong too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was me as well! This line was throwing a YAML SafeLoader error because it wasn't formatted as a k:v pair

@@ -55,4 +58,5 @@ def mock_container():
container_mock = AsyncMock(MockContainerClass)
container_mock.with_label.return_value = container_mock
container_mock.with_exec.return_value = container_mock
container_mock.with_file.return_value = container_mock
Copy link
Contributor

Choose a reason for hiding this comment

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

💎

@ChristoGrab ChristoGrab merged commit 427e8fd into master Sep 3, 2024
40 checks passed
@ChristoGrab ChristoGrab deleted the christo/manifest-only-build-with-components branch September 3, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants