airbyte-ci: format perf improvement + exit 1 when modified files

[alafanechere]

What

Relates to #32935

As we want developers to adopt airbyte-ci format fix locally it has to:

• Be fast
• Exit with 1 when files are modified

How

This PR refactors the format command significantly:

• fix and check run the same command in containers: it will guarantee that formatting errors thrown by check can always be fixed. We also improve caching, as it's the same command a fix following a check will reuse the same docker layer. The single difference between check and fix is that fix exports modified files back to the file system.

• Avoid code duplication at subcommand declaration and centralize the formatters configurations in a single module

• Only exports back to the host the modified files by diffing the original repo dir state and the formatted code. It makes the exportoperation faster

• Exit with 1 status code when fix is modifying files: this is a requirement to wrap this command in a future pre-push hook to avoid pushing unformatted code

• Leverage internal caching in formatters with caching abilities (prettier and black): it leads to a significant boost for prettier: 8seconds vs 30s.

Outcome

airbyte-ci format fix all runs in ~30s once you pulled the images.

NB

• The binary used on this branch by the automatic formatting in the CI will format the unformatted code I added for tests. We should cancel auto format otherwise the tests will fail on master. The committed unformatted code will be ignored by formatters on master once the latest binary is released.
• The tests are not passing because of the same reason: the master binary is used in the CI but I had to make change to airbyte-ci test command to run the formatting tests.

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Dec 8, 2023 10:02am

[alafanechere]

Current dependencies on/for this PR:

• master
  • PR airbyte-ci: format perf improvement + exit 1 when modified files #33097 👈
    • PR airbyte-ci: create a pre-push hook running airbyte-ci format fix all #33170
      • PR [don't merge] testing #33234
      • PR prepush hook test  #33224
      • PR airbyte-ci: disable automatted fix on PRs #33186
    • PR airbyte-ci: test format fix/check all #33137
    • PR airbyte-ci: run format fix all in a pre-push hook #32865

This stack of pull requests is managed by Graphite.

[alafanechere]

We have to add this condition now that airbyte-ci format fix all exits with status code 1 if files were modified.
We'll soon only run check on PRs so this file will likely be removed and replaced by format_check.yml on PRs

[alafanechere]

Replaced by commands.py a level higher.

[postamar]

LGTM modulo fixing a very minor bug that adds *.groovy source files instead of *.grade.
My review is somewhat superficial and doesn't look into the airbyte-ci structure changes too much, but I didn't notice anything shocking. I figure that the correctness of this change is straightforward for you to validate on your end.

[bnchrch]

This is a great improvement!

There are only a few small changes I think we should make.

The only blocking one is how we define our cache keys.

[erohmensing]

Great refactor. Nothing blocking from me!

[alafanechere]

Thanks for the review @erohmensing ! I'll push the your suggestions

If the commands are the same, would we consider putting them back together into one command with a --check arg? definitely not required in any case - but if we're going down the path of making things easier to reason about, and potentially later having a lint or other check that includes linting (or other checks which can't be autofixed)

I created https://github.com/airbytehq/airbyte-internal-issues/issues/2763 . I agree it's indeed something we shall consider. I don't think it's pressing though and we could revisit the interface when we'll want to add linting.

[alafanechere]

@bnchrch about the cache keys:

• We're not talking about NPM or Python (pip) cache here. It's only prettier and black cache.
• I changed the cache volume name for prettier and black to capture their version
• I think it's worth it for prettier: we know the version we install as it's hardcoded in the function building the container.
• It's a bit harder for black as in the execution context we can't determine the exact black version we're going to install. It's determined by poetry.lock (root of the repo) and I find it a bit far fetched to parse this file to check the black version. I took a look at black's code and it appears that their cache folder has __version__ subfolder, so we're rather safe in term of cache/version isolation.

[bnchrch]

One nit but nothing to stand in the way

LGTM!

[CLAassistant]

All committers have signed the CLA.

[alafanechere]

Will merge without CI checks as the binary used for them is the one from master.
I confirmed that the format is 🆗 locally:
airbyte-ci format chell all:

                    Summary of commands results
                    ========================

                    ✅ java

                    ✅ js

                    ✅ license

                    ✅ python

The airbyte-ci tests are also passing locally.

[sentry-io]

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

• ‼️ ExceptionGroup: 3 exceptions were raised in the task group: anyio._backends._asyncio in __aexit__ View Issue
• ‼️ ExceptionGroup: 2 exceptions were raised in the task group: anyio._backends._asyncio in __aexit__ View Issue
• ‼️ QueryError: Unavailable: error reading from server: EOF dagger.api.base in _handle_execute View Issue
• ‼️ QueryError: Unavailable: error reading from server: EOF dagger.api.base in _handle_execute View Issue
• ‼️ QueryError: Unavailable: error reading from server: EOF dagger.api.base in _handle_execute View Issue

_{Did you find this useful? React with a 👍 or 👎}