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

Patch: AJ's pre-commit updates #49846

Merged
merged 10 commits into from
Dec 18, 2024

Conversation

aaronsteers
Copy link
Collaborator

@aaronsteers aaronsteers commented Dec 17, 2024

What

Patches and stacks with

How

  • Replaces airbyte-ci with pre-commit in the format check and format fix CI workflows.
  • Fixes some "excludes" so that intentionally mis-formatted files do not get formatted with --all-files arg is used.
  • Removes all the extra secret refs that were previously sent to airbyte-ci so format check and fix can run without escalated privileges.
  • Allows format checks to run on forks with no privilege escalation. This is preferrable to having a separate CI workflow for forks and non-forks.
  • Allows /format-fix to run on forks (in theory, but will needs to be tested post-merge).

Example Workflow Runs:

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Dec 17, 2024

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

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 0:02am

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@aaronsteers aaronsteers requested a review from a team as a code owner December 17, 2024 22:53
@aaronsteers aaronsteers changed the title Aj/chore/precommit updates Patch: AJ's pre-commit updates Dec 17, 2024
Comment on lines +29 to +31
<excludes>
<exclude>**/non_formatted_code/*</exclude>
</excludes>
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.

Copy link
Contributor

github-actions bot commented Dec 17, 2024

Format-fix job started... Check job output.

🟦 Job completed successfully (no changes).

Copy link
Contributor

github-actions bot commented Dec 17, 2024

Format-fix job started... Check job output.

✅ Changes applied successfully. (aad2e23)

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

@aaronsteers aaronsteers Dec 17, 2024

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

@@ -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.)

^.*?/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

@natikgadzhi natikgadzhi merged commit 2e3fef4 into natik/angry/formatters Dec 18, 2024
11 of 13 checks passed
@natikgadzhi natikgadzhi deleted the aj/chore/precommit-updates branch December 18, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants