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

format: airbyte-ci format fix all on PR / airbyte-ci format check all on master #32491

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Nov 14, 2023

What

I think we want two separate worfklows for format:

  • One which automatically fix format on PRs
  • One which checks format on master and yells in slack if something is not formatted on master

How

  • Split format.yml in two worfklows
  • format_fix.yml: Runs airbyte format fix all on PRs + commit changes
  • format_check.yml : Runs airbyte format check all on merge to master and sends a slack message in case of failure.

Copy link

vercel bot commented Nov 14, 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 Nov 14, 2023 1:52pm

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@alafanechere alafanechere marked this pull request as ready for review November 14, 2023 10:01
@alafanechere alafanechere requested review from postamar and a team November 14, 2023 10:01
@alafanechere alafanechere changed the title format: airbyte-ci format fix all on PR from airbyte-ci format check all on master format: airbyte-ci format fix all on PR / airbyte-ci format check all on master Nov 14, 2023
@@ -1,4 +1,4 @@
name: Check for formatting errors
name: Automatic Formatting on PRs
Copy link
Contributor

Choose a reason for hiding this comment

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

You're going to want to follow the "fix" with a "check". Not everything can always be fixed automatically and formatting errors should be caught before the PR is merged. In fact, that's the DoD for this whole initiative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 715a8e0

@@ -0,0 +1,59 @@
name: Check for formatting errors on head ref
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocking comment but in the near future we're going to want to subsume a larger class of checks than just formatting checks in this workflow, so its name should reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer keeping this name as it is as we don't know yet what's going to be in this "larger class of checks" and we might want to split them in multiple workflows

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point

@alafanechere alafanechere force-pushed the augustin/11-14-format_airbyte-ci_format_fix_all_on_PR_from_airbyte-ci_format_check_all_on_master branch from e656bcb to 715a8e0 Compare November 14, 2023 13:51
@alafanechere alafanechere merged commit 9b94d5e into master Nov 14, 2023
16 checks passed
@alafanechere alafanechere deleted the augustin/11-14-format_airbyte-ci_format_fix_all_on_PR_from_airbyte-ci_format_check_all_on_master branch November 14, 2023 16:04
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