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.

Sign up for GitHub

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

Patch: AJ's pre-commit updates #49846

Merged
merged 10 commits into from
Dec 18, 2024
Merged

Patch: AJ's pre-commit updates #49846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0439ce8
format pre-commit yaml
aaronsteers Dec 17, 2024
a11e9cc
exclude airbyte_ci non-formatted java from spotless formatter
aaronsteers Dec 17, 2024
b163c3f
Update .pre-commit-config.yaml
aaronsteers Dec 17, 2024
ac11980
ci: use pre-commit in format-check
aaronsteers Dec 17, 2024
7c6ee80
ci: use newer ubuntu
aaronsteers Dec 17, 2024
4764973
ci: explicit java and python setup
aaronsteers Dec 17, 2024
168aad8
update ignore
aaronsteers Dec 17, 2024
3bce6b3
ci: update format-fix
aaronsteers Dec 17, 2024
4ff1e85
dummy change to break formatting
aaronsteers Dec 17, 2024
aad2e23
chore: auto-fix lint and format issues
octavia-squidington-iii Dec 17, 2024
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
27 changes: 16 additions & 11 deletions .github/workflows/format-fix-command.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ concurrency:

jobs:
format-fix:
name: "Run airbyte-ci format fix all"
runs-on: ubuntu-latest
name: "Run pre-commit fix"
runs-on: ubuntu-24.04
steps:
- name: Get job variables
id: job-vars
Expand Down Expand Up @@ -67,16 +67,21 @@ jobs:

[1]: ${{ steps.job-vars.outputs.run-url }}

- name: Run airbyte-ci format fix all
uses: ./.github/actions/run-airbyte-ci
continue-on-error: true
# Compare the below to the `format_check.yml` workflow
- uses: actions/checkout@v4
- name: Setup Java
uses: actions/setup-java@v3
with:
distribution: "zulu"
java-version: "21"
- name: Setup Python
uses: actions/setup-python@v5
with:
context: "manual"
gcs_credentials: ${{ secrets.METADATA_SERVICE_PROD_GCS_CREDENTIALS }}
sentry_dsn: ${{ secrets.SENTRY_AIRBYTE_CI_DSN }}
github_token: ${{ secrets.GH_PAT_MAINTENANCE_OCTAVIA }}
subcommand: "format fix all"
dagger_cloud_token: ${{ secrets.DAGGER_CLOUD_TOKEN_CACHE_2 }}
python-version: "3.10"
- name: Run pre-commit
uses: pre-commit/[email protected]
continue-on-error: true
id: format-fix

# This is helpful in the case that we change a previously committed generated file to be ignored by git.
- name: Remove any files that have been gitignored
Expand Down
62 changes: 21 additions & 41 deletions .github/workflows/format_check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,61 +10,41 @@ on:

jobs:
format-check:
# IMPORTANT: This name must match the require check name on the branch protection settings
name: "Check for formatting errors"
# Do not run this job on forks
# Forked PRs are handled by the community_ci.yml workflow
if: github.event.pull_request.head.repo.fork != true
runs-on: tooling-test-small
runs-on: ubuntu-24.04
steps:
- name: Checkout Airbyte (with credentials)
uses: actions/checkout@v4
- uses: actions/checkout@v4
- name: Setup Java
uses: actions/setup-java@v3
with:
ref: ${{ github.head_ref }}
token: ${{ secrets.GH_PAT_APPROVINGTON_OCTAVIA }}
fetch-depth: 1
- name: Run airbyte-ci format check [MASTER]
id: airbyte_ci_format_check_all_master
if: github.ref == 'refs/heads/master'
uses: ./.github/actions/run-airbyte-ci
continue-on-error: true
distribution: "zulu"
java-version: "21"
- name: Setup Python
uses: actions/setup-python@v5
with:
context: "master"
sentry_dsn: ${{ secrets.SENTRY_AIRBYTE_CI_DSN }}
subcommand: "format check all"

- name: Run airbyte-ci format check [PULL REQUEST]
id: airbyte_ci_format_check_all_pr
if: github.event_name == 'pull_request'
uses: ./.github/actions/run-airbyte-ci
continue-on-error: false
with:
context: "pull_request"
sentry_dsn: ${{ secrets.SENTRY_AIRBYTE_CI_DSN }}
subcommand: "format check all"
dagger_cloud_token: ${{ secrets.DAGGER_CLOUD_TOKEN_CACHE_2 }}

- name: Run airbyte-ci format check [WORKFLOW DISPATCH]
id: airbyte_ci_format_check_all_manual
if: github.event_name == 'workflow_dispatch'
uses: ./.github/actions/run-airbyte-ci
continue-on-error: false
python-version: "3.10"
- name: Run pre-commit
uses: pre-commit/[email protected]
id: format-check
with:
context: "manual"
sentry_dsn: ${{ secrets.SENTRY_AIRBYTE_CI_DSN }}
subcommand: "format check all"
dagger_cloud_token: ${{ secrets.DAGGER_CLOUD_TOKEN_CACHE_2 }}
extra_args: --all-files

- name: Match GitHub User to Slack User [MASTER]
if: github.ref == 'refs/heads/master'
if: >
always() && steps.format-check.outcome == 'failure' &&
github.ref == 'refs/heads/master' &&
github.event.pull_request.head.repo.fork == false
id: match-github-to-slack-user
uses: ./.github/actions/match-github-to-slack-user
env:
AIRBYTE_TEAM_BOT_SLACK_TOKEN: ${{ secrets.SLACK_AIRBYTE_TEAM_READ_USERS }}
GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: Format Failure on Master Slack Channel [MASTER]
if: steps.airbyte_ci_format_check_all_master.outcome == 'failure' && github.ref == 'refs/heads/master'
if: >
always() && steps.format-check.outcome == 'failure' &&
github.ref == 'refs/heads/master' &&
github.event.pull_request.head.repo.fork == false
uses: abinoda/slack-action@master
env:
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN_AIRBYTE_TEAM }}
Expand Down
11 changes: 6 additions & 5 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,19 @@ exclude: |
^.*/__init__\.py$|
^.*?/\.venv/.*$|
^.*?/node_modules/.*$|

^.*?/charts/.*$|
^airbyte-integrations/bases/base-normalization/.*$|
^.*?/normalization_test_output/.*$|

^.*?/pnpm-lock\.yaml$|
^.*?/source-amplitude/unit_tests/api_data/zipped\.json$|
Copy link
Contributor

Choose a reason for hiding this comment

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

oooof
thank you


# Generated/test files
^airbyte-ci/connectors/metadata_service/lib/metadata_service/models/generated/.*$|
^.*?/airbyte-ci/connectors/metadata_service/lib/tests/fixtures/.*/invalid/.*$|
^airbyte-ci/connectors/metadata_service/lib/tests/fixtures/.*/invalid/.*$|
^.*?/airbyte-ci/connectors/pipelines/tests/test_format/non_formatted_code/.*$|
Comment on lines +18 to +19
Copy link
Collaborator Author

@aaronsteers aaronsteers Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Formatters seem inconsistent on whether they will accept ^ or ^.*?/ at the start of the path - and this may also have something to do with the relative path or path that the file is cloned at. Adding both ways solves this in the case above that I found was inconsistently applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but I think we can live with that. Ultimately I think we can remove the tests for those unformatted things because, well, those are FORMATTER TESTS

^airbyte-ci/connectors/pipelines/tests/test_format/non_formatted_code/.*$|
^.*?/airbyte-integrations/connectors/destination-.*/expected-spec\.json$
)
Expand All @@ -25,8 +27,7 @@ repos:
hooks:
- id: black
language_version: python3.10
verbose: true
args: ["--verbose"]
args: [--config=pyproject.toml]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The --config-pyproject.toml arg might not be needed, but consistency seemed worthwhile.

Also - I'm okay bringing back verbose if we need it, but was very spammy on my machine when --all-files was passed as an arg.

(I also tried sending an explicit config path to isort - but this had an adverse affect so I reverted it.)


- repo: https://github.com/pycqa/isort
rev: 5.12.0
Expand Down
3 changes: 3 additions & 0 deletions spotless-maven-pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
<includes>
<include>**/*.java</include>
</includes>
<excludes>
<exclude>**/non_formatted_code/*</exclude>
</excludes>
Comment on lines +29 to +31
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was necessary because this file would otherwise be formatted by spotless when running pre-commit with the --all-files flag.

Since spotless doesn't receive individual files from pre-commit, the pre-commit exclude list doesn't apply.

<importOrder />
<eclipse>
<version>4.21</version>
Expand Down
Loading