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

Add github notifications for evals (rebased) #852

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pingiun
Copy link
Member

@pingiun pingiun commented Jan 29, 2021

This replaces #712, because it seemed to me it got stale

I have used this functionality on my own server for a while and it seems to work fine. I adapted the PR created by @pikajude so it's based on the latest master, it uses python instead of perl tests, and it includes the suggestions from @knl

Also in response this point of @knl:

Maybe it would be good to have unified eval context (instead of one being named after the PR), so one could turn on branch protection on the context being successful.

I believe this is possible because you have quite a lot of freedom in the context name via StringInterpolate

If this PR is merged, #712 can be closed

@pingiun
Copy link
Member Author

pingiun commented Jan 29, 2021

I choose to have @pikajude as the co-author instead of committing as them, because I think the commit wouldn't be verified in that case? I'm not sure, let me know if I should change this

@pingiun pingiun force-pushed the github-eval-notifies branch 8 times, most recently from 2886e57 to 1e2c9e3 Compare January 29, 2021 17:50
@pingiun pingiun marked this pull request as draft January 29, 2021 21:12
@pingiun
Copy link
Member Author

pingiun commented Jan 29, 2021

Changing this to draft because I am still trying to get the new test case to succeed

Adapted from NixOS#712

NixOS#712

Co-authored-by: Jude Taylor <jude.taylor@dfinity.org>
@pingiun pingiun marked this pull request as ready for review January 30, 2021 12:04
@pingiun
Copy link
Member Author

pingiun commented Jan 30, 2021

The new check is now succeeding on my hydra instance: https://hydra.technicie.nl/build/499

@pingiun pingiun changed the title Add github notifications for evals Add github notifications for evals (rebased) Feb 3, 2021
@grahamc
Copy link
Member

grahamc commented Feb 24, 2021

I wonder if it would be possible to test this feature similarly to #878, without a separate, and relatively quite heavy VM test?

@pingiun
Copy link
Member Author

pingiun commented Feb 25, 2021

I wonder if it would be possible to test this feature similarly to #878, without a separate, and relatively quite heavy VM test?

Good idea, I'll try it out

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.

None yet

2 participants