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

airbyte-ci format: exclude .gitignored files from format #33249

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Dec 8, 2023

What

Under current implementation airbyte-ci format check/fix can format gitignored files.
So far we've maintained an exclusion rule in DEFAULT_FORMAT_IGNORE_LIST but we don't want to maintain it to match all are git ignore rules.

How

  • Get the list of git ignored files by running git status --ignored --short
  • Exclude DEFAULT_FORMAT_IGNORE_LIST + this list from the files to format.

🚨 User Impact 🚨

Unfortunately git status --ignored is pretty slow (~20s on my laptop) so it'll make the format commands 20s slower...

Copy link

vercel bot commented Dec 8, 2023

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 13, 2023 8:53am

Copy link
Contributor Author

alafanechere commented Dec 8, 2023

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@alafanechere alafanechere changed the title airbyte-ci format: exlcude .gitignored files from format airbyte-ci format: exclude .gitignored files from format Dec 8, 2023
@alafanechere alafanechere marked this pull request as ready for review December 8, 2023 15:41
@alafanechere alafanechere requested review from a team and erohmensing December 8, 2023 15:41
@alafanechere alafanechere force-pushed the augustin/12-08-airbyte-ci_format_exlcude_.gitignored_files_from_format branch from c6cd01c to 8afd4a8 Compare December 11, 2023 09:23
@alafanechere alafanechere changed the base branch from master to augustin/12-11-format-airbyte-ci-init December 11, 2023 09:23
@octavia-squidington-iv octavia-squidington-iv requested a review from a team December 11, 2023 09:28
Base automatically changed from augustin/12-11-format-airbyte-ci-init to master December 11, 2023 09:59
@alafanechere alafanechere force-pushed the augustin/12-08-airbyte-ci_format_exlcude_.gitignored_files_from_format branch from 8afd4a8 to 08c8c32 Compare December 11, 2023 11:33
@alafanechere alafanechere requested a review from a team as a code owner December 11, 2023 11:33
@alafanechere alafanechere changed the base branch from master to augustin/12-11-fix_CDK_format December 11, 2023 11:33
@alafanechere alafanechere mentioned this pull request Dec 11, 2023
@alafanechere alafanechere force-pushed the augustin/12-08-airbyte-ci_format_exlcude_.gitignored_files_from_format branch from 08c8c32 to f658064 Compare December 11, 2023 11:35
@alafanechere alafanechere force-pushed the augustin/12-08-airbyte-ci_format_exlcude_.gitignored_files_from_format branch from 24c9ceb to 775f0f2 Compare December 11, 2023 19:35
@alafanechere alafanechere requested review from a team as code owners December 11, 2023 19:35
@alafanechere alafanechere changed the base branch from augustin/12-11-fix_CDK_format to master December 11, 2023 19:35
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

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

I'm glad this works! There's a lot of room to simplify this implementation, however.

# Add all files to the git index
.with_exec(["git", "add", "."])
# Commit all files
.with_exec(["git", "commit", "--message", "Commit for formatting"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe that you need to commit anything for git clean to do its thing, in which case you can remove everything in between init and clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't commit the files they will be considered as untracked files - hence the commit

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true that they will be considered as untracked files, however, that does not affect how clean behaves. See for yourself, try it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also surprised that it works, mind you, so I looked into it. The -X flag for clean is the reason it works: https://git-scm.com/docs/git-clean

airbyte-ci/connectors/pipelines/pipelines/helpers/git.py Outdated Show resolved Hide resolved
Comment on lines 102 to 108
# This is needed to avoid git complaining about the user not being set
.with_exec(["config", "user.email", "ci@airbyte.io"])
.with_exec(["config", "user.name", "Airbyte CI"])
# Add all files to the git index
.with_exec(["add", "."])
# Commit all files
.with_exec(["commit", "--message", "Commit for formatting"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This is needed to avoid git complaining about the user not being set
.with_exec(["config", "user.email", "ci@airbyte.io"])
.with_exec(["config", "user.name", "Airbyte CI"])
# Add all files to the git index
.with_exec(["add", "."])
# Commit all files
.with_exec(["commit", "--message", "Commit for formatting"])

# Commit all files
.with_exec(["commit", "--message", "Commit for formatting"])
# Remove all gitignored files
.with_exec(["clean", "-dfX"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.with_exec(["clean", "-dfX"])
.with_exec(["clean", "-dfqX"])

The -q option makes for less noise in stdout, apparently.

@alafanechere alafanechere force-pushed the augustin/12-08-airbyte-ci_format_exlcude_.gitignored_files_from_format branch from 65b84fc to 823514c Compare December 13, 2023 08:53
@alafanechere alafanechere merged commit 098a285 into master Dec 13, 2023
21 checks passed
@alafanechere alafanechere deleted the augustin/12-08-airbyte-ci_format_exlcude_.gitignored_files_from_format branch December 13, 2023 09:33
tmathew0309 pushed a commit to tmathew0309/airbyte that referenced this pull request Jan 12, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants