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

CI: Allow format fix on PRs from forks #37393

Merged
merged 7 commits into from
Apr 24, 2024

Conversation

aaronsteers
Copy link
Collaborator

@aaronsteers aaronsteers commented Apr 18, 2024

This modified the format_fix CI workflow as follows:

  • Accept PR number as input, in place of a git ref.
  • Operate within forks as well as the main repo.

I also connected to the /format-fix slash command, and added feedback comments to the PR being operated on.

Copy link

vercel bot commented Apr 18, 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 Apr 24, 2024 5:21pm

@aaronsteers aaronsteers changed the title CI: allow format fix on forks CI: Allow format fix on forks Apr 18, 2024
@aaronsteers aaronsteers changed the title CI: Allow format fix on forks CI: Allow format fix on PRs from forks Apr 18, 2024
@natikgadzhi
Copy link
Contributor

@aaronsteers tell me when you'd like a review on this. Looks sane.

@aaronsteers
Copy link
Collaborator Author

aaronsteers commented Apr 22, 2024

@natikgadzhi - This could be reviewed+merged as-is, but there are two outstanding questions here:

  1. Is it okay to connect back to the slash command? (Maybe we wait until next iteration?)
    • We can't test slash command usage until merge, unfortunately, due to hard mapping to master.
  2. Is it okay to auto-add add functionality to comment on the PR with the auto-format statuses?

We have code to copy-paste which gives this functionality, but it also makes the PR larger, more surface area to review.

UPDATE: I've toggled to 'ready for review' status, in the context of the above two questions.

FWIW, I've tested successfully on a couple PRs. (It can be run from the manual workflow dispatch - selecting this branch and then entering any PR number.)

@aaronsteers aaronsteers marked this pull request as ready for review April 22, 2024 18:39
@aaronsteers aaronsteers requested a review from a team as a code owner April 22, 2024 18:40
@natikgadzhi
Copy link
Contributor

Let's add this to review bucket with @alafanechere for Tuesday. My expectation is that tomorrow we decide between:

  • OK to merge as is with slash commands and schedule improvements for 1-2 months in front of us
  • We agree to rework to using buttons, AND there is a clear plan to get them to work this week.
  • We can synchronously tweak this to use an approach we like, so we merge during the sync ;)

@aaronsteers
Copy link
Collaborator Author

Per meeting today, @alafanechere to review. Hopefully merge today or tomorrow 🚀

@aaronsteers aaronsteers merged commit 1c187e9 into master Apr 24, 2024
31 checks passed
@aaronsteers aaronsteers deleted the aj/ci/allow-format-fix-on-forks branch April 24, 2024 19:11
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