-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
build: experimental PR speed improvement: filter PR change folder #3477
base: master
Are you sure you want to change the base?
Conversation
…h we run tests and image builds. No need to do so for docs / example only changes Signed-off-by: Dave Lee <[email protected]>
✅ Deploy Preview for localai ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@mudler This one will probably need a manual approval / merge on your part: I think there's github repo-side changes you'll need to make as the project admin to support this - let me know how I can help figure out what we need to allow conditionally-required checks. I may be wrong here - I'm also seeing some info that indicates a skipped check may automatically count as passed, so I think the true test here is to see what happens upon approval, and then fix or revert quickly based on that result. It's just somewhat weird to see "14 expected" down there 😆 |
I'd really like to move in this direction, however to my knowledge there is no way for GitHub to set 'conditionally' required checks unless I'm missing something.. afaik GitHub just let you select required status checks, and the only way around it would be to mark them to pass as part of the test itself by conditionally returning 'success' even if no real tests are being run; ignoring workflow would impede the PR to be merged as status checks can be just set as 'required' |
@mudler I missed this while reading docs yesterday - it's absolutely the case that skipped checks count as passed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 💯
Interesting. It has not auto-merged. It seems that based on https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/using-jobs-in-a-workflow#defining-prerequisite-jobs we'll need to add a "report status" stage at the very end with Given that I'm not sure exactly what you've set up in the project settings, would it be more helpful to "rename" our current checks, and use the existing names for the "required status reporting" stage? Or would you prefer I create simpler "status-check-result" stages for each workflow and leave the existing ones as-is? The latter is probably a bit cleaner and simpler to maintain, but will require some changes on your side to update what checks are and are not required. |
Signed-off-by: Dave Lee <[email protected]>
huh, that's very annoying :( also because if we start to do changes in the workflow we have to keep an eye on updating that step, that's really annoying that GHA doesn't have native support for such a common problem.
No problem with that 👍 we can force merge at that point and I'll update the settings |
Signed-off-by: Dave Lee <[email protected]>
Signed-off-by: Dave Lee <[email protected]>
Signed-off-by: Dave Lee <[email protected]>
Signed-off-by: Dave Lee <[email protected]>
Signed-off-by: Dave Lee <[email protected]>
Looks like another draft is in order. I'll update this soon. |
There's no need to run tests or build images when the changes are docs-only or exclusive to the
examples/**
directoriesIn particular I expect this to speed up the numerous dependabot PRs to python dependencies down in examples.
Netlify should probably be configured in a similar way, but I am not sure how to do that so one thing at a time.
In the future, it may be possible to take this farther and only run extras tests when grpc /
core/backend
code changes... but that is also out of scope for now.