Better intellij dev experience #28817

edgao · 2023-07-27T23:11:10Z

I'm sure there's more cleanup that could happen in the plugin file. But presumably we want to just nuke the entire airbyteDocker plugin eventually, so not going to bother.

airbyteDocker now explicitly declares build/distributions as an input directory, which I believe is what most (all?) java connectors copy into their docker image. This probably misses a few edge cases, but given that this only matters for running locally in intelllij - I think that's OK.

(confirmed locally that running tests twice in a row skips airbyteDocker correctly)

github-actions · 2023-07-27T23:11:31Z

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.

evantahler · 2023-07-28T00:04:39Z

Tagging @alafanechere - this completes the circle 🟢!

evantahler · 2023-07-28T00:06:03Z

@edgao so like... does it work? Can we get a loom of you using it?

run docker system prune -a
run a test in inteliJ (time it)
run that test in inteliJ a second time (is it faster?)

octavia-squidington-iii · 2023-07-28T00:16:23Z

destination-snowflake test report (commit `4cae645174`) - ✅

⏲️ Total pipeline duration: 63mn12s

Step	Result
Validate airbyte-integrations/connectors/destination-snowflake/metadata.yaml	✅
Connector version semver check	✅
QA checks	✅
Build connector tar	✅
Build destination-snowflake docker image for platform linux/x86_64	✅
Build airbyte/normalization-snowflake:dev	✅
./gradlew :airbyte-integrations:connectors:destination-snowflake:integrationTest	✅

🔗 View the logs here

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=destination-snowflake test

edgao · 2023-07-31T16:04:14Z

https://www.loom.com/share/69e8677d903d48d6821939b2e1cd96ea

it does run successfully, but I think airbyte-ci isn't actually caching successfully on my laptop. Ran it twice in succession and it took >9 minutes both times (nearly 10 minutes on the first run).

the old airbyteDocker was significantly faster, but I think that's a bit misleading - it also wasn't reliably building new images, so I was manually running airbyte-ci before tests anyway. I.e. I think this is still an improvement for local dev.

I'll try and find a way to disable this on CI for now. The CI test run on this branch took about 10 minutes longer to run integrationTest (54mn39s vs 44mn04s on the latest nightly)... which might indicate more general caching issues, actually

edgao · 2023-07-31T16:11:29Z

also, if you want to try this out locally - if you haven't rebooted since switching to the poetry-based airbyte-ci install:

restart intellij
kill all your gradle daemon processes

otherwise you might get weird issues where they can't find airbyte-ci

octavia-squidington-iii · 2023-08-09T01:18:27Z

destination-snowflake test report (commit `03294ed444`) - ✅

⏲️ Total pipeline duration: 41mn39s

Step	Result
Validate airbyte-integrations/connectors/destination-snowflake/metadata.yaml	✅
Connector version semver check	✅
QA checks	✅
Build connector tar	✅
Build destination-snowflake docker image for platform linux/x86_64	✅
Build airbyte/normalization-snowflake:dev	✅
./gradlew :airbyte-integrations:connectors:destination-snowflake:integrationTest	✅

🔗 View the logs here

☁️ 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=destination-snowflake test

alafanechere

This is great and a first nice step in sunsetting airbyteDocker right?
Please note my comment; LGTM otherwise.
Would we want to handle the airbyte-ci installation from gradle? (might be overkill ATM for local gradle execution)

buildSrc/src/main/groovy/airbyte-docker.gradle

alafanechere · 2023-08-09T09:35:25Z

I'll try and find a way to disable this on CI for now. The CI test run on this branch took about 10 minutes longer to run integrationTest (54mn39s vs 44mn04s on the latest nightly)... which might indicate more general caching issues, actually

When airbyte-ci runs integration test or build it's using a gradle command with -x airbyteDocker : I think it means that on the CI airbyte-ci won't call itself (which is the desired behavior right?).

edgao · 2023-08-09T18:30:13Z

I don't think this is mergeable until dagger/gradle (idk which) caching works better :/ Intellij runs destination-foo:airbyteDocker anytime you run anything inside test-integration... even if that test doesn't actually need the docker image (e.g. BigQuerySqlGeneratorIntegrationTest). Currently that iteration loop is really tight (because airbyteDocker terminates quickly, and it doesn't matter that it doesn't work); adding a 9-minute airbyte-ci build would be really painful

maybe we should have test-integration for tests that need secrets / infra but not the docker image, and test-acceptance for anything that depends on the docker image? but I think that's a bigger project

or maybe we should just delete airbyteDocker completely? and tell developers to manually run airbyte-ci before running tests from intellij. I think that's basically what we have today, except without the "sometimes intellij automatically builds the docker images, but extremely unreliably" behavior

evantahler · 2023-08-09T22:16:00Z

@edgao I think we can keep this task fast when nothing has changed if we modify the isUpToDate() method. I think this method is expecting project.rootProject.imageToHash.get to have been set by the build command, which are no longer doing. We could set that after shelling out to airbyte-ci, or be lazier and skip the hash check, and just be OK if the image exists at all

edgao · 2023-08-10T22:47:58Z

I dropped the uptodate check entirely b/c I thought airbyte-ci would handle it 🤦

I never quite figured out what the imageToHash thing actually does, and suspect it over and/or undercaches things... maybe gradle has something for "run this task only if compileJava did anything"? Which probably works >95% of the time (i.e. it misses changes to shell scripts/etc, but we basically never change those files anyway, and we can always come back to this))

edgao · 2023-08-10T23:58:09Z

airbyteDocker now explicitly declares build/distributions as an input directory, which I believe is what most (all?) java connectors copy into their docker image. This probably misses a few edge cases, but given that this only matters for running locally in intelllij - I think that's OK. PTAL!

(confirmed locally that running tests twice in a row skips airbyteDocker correctly)

alafanechere · 2023-08-11T08:01:32Z

@edgao you're right considering the airbyte-ci gradle caching does not properly work.
I logged this issue to improve it #28983
Conor made it work in aircmd (the with_gradle function).

edgao · 2023-08-11T18:43:38Z

connectors base build failed on CI:

2023-08-11T17:59:36.0531571Z 1: Task failed with an exception.
2023-08-11T17:59:36.0532505Z -----------
2023-08-11T17:59:36.0532899Z * What went wrong:
2023-08-11T17:59:36.0533604Z Execution failed for task ':airbyte-integrations:connectors:destination-bigquery:airbyteDocker'.
2023-08-11T17:59:36.0534290Z > A problem occurred starting process 'command 'airbyte-ci''

but no other information :( will keep poking at this

edgao · 2023-08-11T22:52:36Z

uhhhh I'm going to just remove the connectors from the connectors_base sub build. settings.gradle claims they're only needed for normalization integration tests, which we're not running anymore.

github-actions · 2023-08-17T22:00:11Z

destination-bigquery test report (commit `197f8245d4`) - ✅

⏲️ Total pipeline duration: 32mn19s

Step	Result
Validate airbyte-integrations/connectors/destination-bigquery/metadata.yaml	✅
Connector version semver check	✅
QA checks	✅
Build connector tar	✅
Build destination-bigquery docker image for platform linux/x86_64	✅
Build airbyte/normalization:dev	✅
./gradlew :airbyte-integrations:connectors:destination-bigquery:integrationTest	✅

🔗 View the logs here

☁️ 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=destination-bigquery test

github-actions · 2023-08-17T22:45:57Z

destination-snowflake test report (commit `197f8245d4`) - ✅

⏲️ Total pipeline duration: 45mn34s

Step	Result
Validate airbyte-integrations/connectors/destination-snowflake/metadata.yaml	✅
Connector version semver check	✅
QA checks	✅
Build connector tar	✅
Build destination-snowflake docker image for platform linux/x86_64	✅
Build airbyte/normalization-snowflake:dev	✅
./gradlew :airbyte-integrations:connectors:destination-snowflake:integrationTest	✅

🔗 View the logs here

☁️ 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=destination-snowflake test

airbyteDocker shells out to airbyte-ci

4cae645

edgao requested a review from a team as a code owner July 27, 2023 23:11

octavia-squidington-iii added area/connectors Connector related issues connectors/destination/snowflake labels Jul 27, 2023

jbfbell approved these changes Aug 2, 2023

View reviewed changes

Merge branch 'master' into edgao/airbytedocker_airbyte_ci

03294ed

alafanechere reviewed Aug 9, 2023

View reviewed changes

buildSrc/src/main/groovy/airbyte-docker.gradle Outdated Show resolved Hide resolved

Merge branch 'master' into edgao/airbytedocker_airbyte_ci

470296c

edgao marked this pull request as draft August 9, 2023 20:49

edgao mentioned this pull request Aug 10, 2023

hack for intellij: make integrationTestJavaClasses depend on airbyteDocker #29348

Closed

edgao added 2 commits August 10, 2023 16:45

explicit dependencies

c1799a0

disable tui

ab57ea5

edgao marked this pull request as ready for review August 10, 2023 23:58

add comment back

8442d5e

edgao changed the title ~~airbyteDocker shells out to airbyte-ci~~ Better intellij dev experience Aug 11, 2023

This comment was marked as outdated.

Sign in to view

also remove airbyteDocker dep from bigquery

8cf67b4

octavia-squidington-iii added the connectors/destination/bigquery label Aug 11, 2023

This comment was marked as outdated.

Sign in to view

exclude specific modules

e1b4fdc

This comment was marked as outdated.

Sign in to view

edgao marked this pull request as draft August 11, 2023 22:52

remove connectors from base build

1cde8d0

edgao marked this pull request as ready for review August 11, 2023 23:25

This comment was marked as outdated.

Sign in to view

Merge branch 'master' into edgao/airbytedocker_airbyte_ci

197f824

postamar mentioned this pull request Sep 6, 2023

improve the gradle build correctness and performance #30204

Closed

postamar mentioned this pull request Sep 13, 2023

gradle,airbyte-ci: replace ‘airbyte-docker’ plugin in connectors with something that delegates to airbyte-ci #30403

Closed

postamar mentioned this pull request Sep 25, 2023

airbyte-ci,gradle: replace airbyte-docker with airbyte-ci #30743

Merged

edgao closed this Nov 13, 2023

edgao deleted the edgao/airbytedocker_airbyte_ci branch November 13, 2023 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better intellij dev experience #28817

Better intellij dev experience #28817

edgao commented Jul 27, 2023 •

edited

Loading

github-actions bot commented Jul 27, 2023 •

edited by edgao

Loading

evantahler commented Jul 28, 2023

evantahler commented Jul 28, 2023

octavia-squidington-iii commented Jul 28, 2023

edgao commented Jul 31, 2023

edgao commented Jul 31, 2023

octavia-squidington-iii commented Aug 9, 2023

alafanechere left a comment

alafanechere commented Aug 9, 2023 •

edited

Loading

edgao commented Aug 9, 2023

evantahler commented Aug 9, 2023

edgao commented Aug 10, 2023

edgao commented Aug 10, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

alafanechere commented Aug 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

edgao commented Aug 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

edgao commented Aug 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

Better intellij dev experience #28817

Better intellij dev experience #28817

Conversation

edgao commented Jul 27, 2023 • edited Loading

github-actions bot commented Jul 27, 2023 • edited by edgao Loading

Before Merging a Connector Pull Request

evantahler commented Jul 28, 2023

evantahler commented Jul 28, 2023

octavia-squidington-iii commented Jul 28, 2023

destination-snowflake test report (commit 4cae645174) - ✅

edgao commented Jul 31, 2023

edgao commented Jul 31, 2023

octavia-squidington-iii commented Aug 9, 2023

destination-snowflake test report (commit 03294ed444) - ✅

alafanechere left a comment

Choose a reason for hiding this comment

alafanechere commented Aug 9, 2023 • edited Loading

edgao commented Aug 9, 2023

evantahler commented Aug 9, 2023

edgao commented Aug 10, 2023

edgao commented Aug 10, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

alafanechere commented Aug 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

edgao commented Aug 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

edgao commented Aug 11, 2023

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot commented Aug 17, 2023

destination-bigquery test report (commit 197f8245d4) - ✅

github-actions bot commented Aug 17, 2023

destination-snowflake test report (commit 197f8245d4) - ✅

edgao commented Jul 27, 2023 •

edited

Loading

github-actions bot commented Jul 27, 2023 •

edited by edgao

Loading

destination-snowflake test report (commit `4cae645174`) - ✅

destination-snowflake test report (commit `03294ed444`) - ✅

alafanechere commented Aug 9, 2023 •

edited

Loading

edgao commented Aug 10, 2023 •

edited

Loading

destination-bigquery test report (commit `197f8245d4`) - ✅

destination-snowflake test report (commit `197f8245d4`) - ✅