-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
[RFC 0056] CI all Nix PRs #56
Changes from all commits
20ab913
da00dab
317f7ee
cca84af
6fe117e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
--- | ||
feature: ci-all-nix-prs | ||
start-date: 2019-10-30 | ||
author: John Ericson (@Ericson2314) | ||
co-authors: (find a buddy later to help our with the RFC) | ||
shepherd-team: (names, to be nominated and accepted by RFC steering committee) | ||
shepherd-leader: (name to be appointed by RFC steering committee) | ||
related-issues: (will contain links to implementation PRs) | ||
--- | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Build all Nix PRs in CI. | ||
Do not merge any PR until it passes CI. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
There is a [famous blog post][blog-post] about how everyone is sloppy and doing CI wrong. | ||
This isn't just bad for releasing software smoothly, but also increases the burden for new contributors because it is harder to judge the correctness of PRs at a glance (is it broken? Did I break it?). | ||
I personally find it harder to contribute, I have to worry about double checking all my work on platforms I don't have as-easy access to, like Darwin. | ||
|
||
We cannot yet do this for all of Nixpkgs, sadly, due to resource limits. | ||
But, there is no reason we cannot do it for Nix itself. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
Optional first step: we can set up OfBorg to build all PRs. | ||
|
||
Set up Hydra declarative jobsets to build all approved Nix PRs. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to define what approved means here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I have no preference. We just need something to solve the small security problems. |
||
This might involve extending Hydra somewhat. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you clarify how Hydra would have to be extended? I can imagine for example giving it access to GitHub's API, to know whether a PR is "approved". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be via github approval, or a github comment, whichever is easier. |
||
Those with merge access should be instructed not to merge a PR until CI passes. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can actually be enforced on GitHub, right? So why not make it part of the detailed design? |
||
Merge master into PRs or rebase before merge as a crude stop-gap to avoid master becoming an untested tree due to a merge commit. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could Hydra build the PR and also build the merge commit of the PR into master? |
||
|
||
If Hydra gains the ability to keep master always working obviating the manual steps above beyond the PR jobs, use that ability. | ||
|
||
If a new CI is used, ensure that is also keeps master in an always-building state. | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
More process to follow. | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
1. Merely build all PRs with OfBorg. | ||
This is still far better than the status quo, but has the disadvantage that master must still be rebuilt as OfBorg and Hydra do not share a cache. | ||
|
||
2. Merely build all approved PRs with Hydra, but maintainers are still allowed to merge broken ones / not take care to avoid untested merge commits. | ||
This is better still, but master could still be broken, even if only working branches are merged due to the merges not being tested. | ||
|
||
3. Build all PRs with Hydra. | ||
This was my original proposal, which has the benefit of not requiring new Hydra features. | ||
Unfortunately there is a slight security risk. | ||
While we generally trust Nix sandboxing---Nixpkgs PR reviewers do not do a full security audit or anything close---fixed output derivations have no network sandboxing. | ||
This means a mischievous PR could is free to do some work and communicate its result to the outside world, rather than have it be lost for ever. | ||
So yes, people could mine crypto-currency or something from within their filesystem-but-not-network sandbox. | ||
Worse, conceivably through some side channel that Linux namespaces do not guard, a rouge fixed-output derivation could try to slowly exfiltrate secrets or something. | ||
|
||
Only building approved PRs is a crude, but probably adequate workaround. | ||
A rogue fixed-output derivation should be much harder to hide than arbitrary malicious code, especially as the Nix code of any PR should be understandable to our reviewers, and much smaller than the C++. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
Nothing. | ||
|
||
# Future work | ||
[future]: #future-work | ||
|
||
- Remove the manual parts of the "it's not rocket science" process. | ||
This means that either one can only merge when it would be a "fast forward" merge, or CI does the merging for your. | ||
This enforces that the tip of master is always building and cached, even as it is pushed to a new commit, race free. | ||
|
||
- Also apply process to other smaller official repos, like `cabal2nix`. | ||
|
||
[blog-post]: https://graydon2.dreamwidth.org/1597.html |
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.
Another motivation is that broken tests are not always the author's fault as master often turns red. Meaning that the author now has to checkout master to see this is a new error or not. If master is red, the author now has to either:
a. find a working commit in the history. If there are any merge conflict this might not be an option.
b. try and fix master himself. This might be completely unrelated work, meaning the author has to load and understand a new context.
c. wait or pester other contributors to fix master so they can continue their work.
Basically the later an error gets caught, the more expensive it becomes to fix.
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.
We already have something like "rocket science" CI if you treat "nixpkgs-channels" as the source code. The channels will only update if the branch passes all of its blocking tests. This accomplishes the same goal as mentioned in the post and avoids the crazy costs that come with "rocket science" CI.
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.
The problem with the way we use Hydra for nixpkgs-channels is that if a broken commit is ever merged to nixpkgs, everything that gets merged after the broken commit also gets blocked from making it to nixpkgs-channels until the problem is fixed.
Don't get me wrong: It's definitely much better than some "CI" systems I've used, but it's not quite as good as the first one I used, which only blocks the broken commit until it's fixed, while allowing other (passing) commits to be merged.