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] Work around diff size limit for static checks #89944

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 27, 2024

See:

Includes that PR just to verify it will solve the limit issue

Edit: Works as expected with the fetch, see here

Affects (as far as I can find):

@AThousandShips AThousandShips added this to the 4.3 milestone Mar 27, 2024
@AThousandShips AThousandShips marked this pull request as ready for review March 27, 2024 14:44
@AThousandShips AThousandShips requested a review from a team as a code owner March 27, 2024 14:44
@AThousandShips AThousandShips changed the title Pr fetch fix [CI] Work around diff size limit for static checks Mar 27, 2024
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 27, 2024
@AThousandShips
Copy link
Member Author

Turns out this doesn't list all the files, but it at least works, as opposed to the other case, unsure how it can be solved better otherwise

@AThousandShips AThousandShips marked this pull request as draft March 27, 2024 15:04
@Chubercik
Copy link
Contributor

Turns out this doesn't list all the files, but it at least works, as opposed to the other case, unsure how it can be solved better otherwise

Maybe we should fall back to this solution if the one using diff errors out?

If so it'd be a good idea to somehow signal that this fallback has happened.

@AThousandShips
Copy link
Member Author

I think I have an alternative solution, will push soon

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 27, 2024

There, new and better solution works, using the number of commits in the PR from the action, should work well now, will rebase on top of updated PR and check properly

Tried to dig into the format and data in the actions but so far I didn't find an explicit list of files, so using a trick with just getting HEAD~n where n is the number of commits in a PR (ignore the previous typo making it only work by accident)

@Chubercik
Copy link
Contributor

Chubercik commented Mar 27, 2024

Can black leave me alone? 😩

One sec, I'll push a fix.

Done.

@AThousandShips

This comment was marked as outdated.

@AThousandShips AThousandShips force-pushed the pr_fetch_fix branch 2 times, most recently from 7d24d02 to 69dc279 Compare March 27, 2024 15:56
@AThousandShips AThousandShips force-pushed the pr_fetch_fix branch 2 times, most recently from 452b4ae to 62531ea Compare March 27, 2024 16:04
@AThousandShips
Copy link
Member Author

Sorry for the false start, reverted to the github.event.pull_request.commits based method which always gives the complete PR data, ensuring it covers everything, now ready for review

@akien-mga akien-mga merged commit d2f9245 into godotengine:master Mar 27, 2024
16 of 24 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the pr_fetch_fix branch March 27, 2024 19:08
@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 27, 2024

It seems to break a bit with merge commits, will try fix

Edit: False alarm, it just doesn't print the changed files properly, it does check them, so it's all okay

Edit 2: Seems it wasn't a false alarm and it just worked partially, new fix up and ready

Un-cherry-picked this and the new PR should be cherry-picked instead (manually or otherwise, no need to have a partial fix in there)

@AThousandShips AThousandShips removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 28, 2024
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