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

Source File: Update Dockerfile to latest Airbyte template #11969

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

Phlair
Copy link
Contributor

@Phlair Phlair commented Apr 13, 2022

What

As a POC part of #6455, I'm going to have source-file-secure use the new intra-repo dependency method to inherit from source-file.

This PR simply updates source-file's Dockerfile to our current layout so that this will actually work. This is because the intra-repo dependency method (at least the template example) relies on py libs & module code files from the dependency module being in a particular directory in its docker image.

note: this isn't going to be a necessity, the paths can be changed to suit the dependency docker image locations, however I want this as a look-at-me example in the docs so best to use standard layout.

also, updated the spec.json so it passes acceptance tests

@Phlair Phlair requested a review from sherifnada April 13, 2022 13:54
@github-actions github-actions bot added the area/connectors Connector related issues label Apr 13, 2022
@Phlair
Copy link
Contributor Author

Phlair commented Apr 13, 2022

/test connector=connectors/source-file

🕑 connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/2161626379
✅ connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/2161626379
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              285    106    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  886    259    71%
Name                      Stmts   Miss  Cover
---------------------------------------------
source_file/__init__.py       2      0   100%
source_file/client.py       237     37    84%
source_file/source.py        50     35    30%
---------------------------------------------
TOTAL                       289     72    75%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.7/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
======================== 20 passed, 1 skipped in 23.87s ========================

@Phlair Phlair self-assigned this Apr 13, 2022
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

I would leave comments in the Dockerfile explaining why we have this unique setup there and not in other connectors (may end up being a link to the docs you eventually write)

the alternative here is to leave the dockerfile as is, do what file secure is currently doing, but that limits us to only one such intra repo dependency right? I think the explicit approach is better fwiw

@Phlair
Copy link
Contributor Author

Phlair commented Apr 13, 2022

@sherifnada good shouts

I would leave comments in the Dockerfile explaining why we have this unique setup

I'll do that in the files-secure Dockerfile since that will be the one with the extra dependency lines. This one is now just matching our general format.

but that limits us to only one such intra repo dependency right?

exactly, which is fine and functional in this case as is, but I'm altering these to create an example for the new method (allowing multi) which I can link to in the docs.

@Phlair
Copy link
Contributor Author

Phlair commented Apr 13, 2022

/publish connector=connectors/source-file

🕑 connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/2161966905
🚀 Successfully published connectors/source-file
🚀 Auto-bumped version for connectors/source-file
✅ connectors/source-file https://github.com/airbytehq/airbyte/actions/runs/2161966905

RUN apt-get update
WORKDIR /airbyte/integration_code
COPY setup.py ./
RUN pip install --prefix=/install .
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just install this in /usr/local instead of doing the whole multi stage thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've emulated our current generator template.

afaia, the benefit to multi-staging like this is that any unneeded build dependencies, artifacts aren't in the final Docker image, potentially reducing image size (although I haven't tested how much difference it's actually making in the Airbyte case)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets April 13, 2022 15:08 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets April 13, 2022 15:08 Inactive
@Phlair Phlair merged commit 26aa43b into master Apr 13, 2022
@Phlair Phlair deleted the george/intrarepo-py-deps branch April 13, 2022 15:40
suhomud pushed a commit that referenced this pull request May 23, 2022
* update to our newer Dockerfile format

* fixes to source-file spec

* auto-bump connector version

Co-authored-by: Octavia Squidington III <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants