<vcunat>
Maybe it's some newer feature. I feel that the competition from GitLab really helps, and they often "steal" each other's ideas if they prove to be useful.
<timokau[m]>
Why don't we protect master yet by the way? I read somewhere that ofBorg's `build-then-merge` is a blocking factor, but what stops contributors from going through PRs now?
<vcunat>
timokau[m]: all our protected branches (including master) are so far only protected against non-fast-forward pushes and deletion
<vcunat>
(I believe)
<timokau[m]>
Oh, I thought branch protection == only PRs
<vcunat>
Forcing all go through PRs is certainly a thing to consider.
<jtojnar>
timokau[m]: going through PRs without build-then-merge is annoying for small fixes
<timokau[m]>
Still, do you know why we don't do that yet? Seems to me like it would be a big advantage with minimal inconvenience.
<vcunat>
And it was considered in connection with Borg some time ago, but not enabled (yet).
<vcunat>
BTW, going through PR by itself doesn't really enforce anything.
<timokau[m]>
jtojnar: You could still immediately merge them. At least ofBorg might still show eval failures, predicted number of rebuilds and whatever other CI there is besides builds.
<timokau[m]>
Next step would be requiring ofBorg approval, that would need `build-then-merge`. But I think just going through PRs would already provide some benefit.
<jtojnar>
well, if I wanted to make use of any of these, I would have to wait (# of rebuilds still takes several minutes usually) – that is the annoyance I am talking about
<jtojnar>
for small changes that you are reasonably certain that they evaluate it is annoying
<vcunat>
yes, it does have such disadvantages, and the history might be harder to read in some cases
<gchristensen>
and it adds a lot of PRs which otherwise wouldn't happen, possibly making it harder to see PRs which actually need review
<gchristensen>
I'm on board with it, but we need to tread carefully
<jtojnar>
sure, occasionaly something slips, but I do not think it happens so often to forbid direct pushes, until we have auto-merge
<gchristensen>
I'd like ofborg's eval to be much faster, too
<timokau[m]>
Alright looks like the consensus is against it for now, just throwing my opinion out there :)
<vcunat>
Ah, yes, this command that merges when checks pass.
sir_guy_carleton has joined #nixos-dev
<gchristensen>
I'm very much _for_ it, but I want it to go smoothly and not be a giant burden
ma27 has joined #nixos-dev
<timokau[m]>
For the "huge number of PRs" problem I would like to have a tagging workflow (would also need bot support, see ofborg #164). Then we would only need to look at number of `needs:review` PRs.
<timokau[m]>
gchristensen: Yeah better safe than sorry^^
<gchristensen>
yeah, the way I see it is we have one, maybe two chances to turn it on and have it go well before its a very hard thing to try again
<timokau[m]>
You think that "public opinion" would go the wrong way if we enable it too early?
<gchristensen>
yeah
<timokau[m]>
Maybe you're right, can't hurt much to wait a while longer.
<gchristensen>
if we make it too annoying it won't be fun to contribute anymore, and then people who don't have to won't
<srhb>
gchristensen: Would it be really hard to have a parallel branch that is always merged-on-build by ofborg for a while? I realize it'll diverge quickly, but it would be interesting to see how much breakage there is.
<gchristensen>
what happens if a commit breaks the series? :)
<srhb>
We'd have to reset it occasionally to master
<gchristensen>
yeah, I think if we had far fewer people that could work, but the rate of change is too high for it IMO
<srhb>
OK. It was mostly to see how it'd go over a short term.
<srhb>
We could also consider a "staging-like" branch that has automerging on, but requires manual merges into master
<srhb>
And then some people would use that instead of master to test automerging specfically
<gchristensen>
that seems like a cool idea
<srhb>
I think we're enough people that care about this that it might be doable, if not completely large-scale
<samueldr>
I like the idea of not-master-but-almost branch to track what would currently be direct-to-master commits
<gchristensen>
my concern is around what happens if one of the commits breaks the chain, how is that dealt with? seems like committing to that branch could cause hard people-problems
<samueldr>
is the cost worse than breaking master?
<samueldr>
(not being contrarian here)
<samueldr>
if not-master breaks, master can (in theory) still build and updates merged through PRs still will go their merry way, right?
<srhb>
The people-fix is force pushing a --reset hard master at not-master
<gchristensen>
seems we need to quantify the cost of a master break ):
<gchristensen>
:)
<samueldr>
yes, this wasn't hand-waving the argument, I really want to understand the benefits too of the direct-to-master commits
biopandemic has joined #nixos-dev
<samueldr>
and risks
<gchristensen>
a benefit are trivial commits are trivial to apply and are fast to get done
<infinisil>
I get your point, and it's a valid point. Combining them however has the benefit of not introducing any duplicates automatically
<infinisil>
And if we had proper ADT's for the module system this would be a bit better
<srhb>
infinisil: I hadn't thought of that. You're right.
orivej has quit [Ping timeout: 268 seconds]
<infinisil>
The current implementation could still be improved though: The specPaths collection can be moved into the submodule (as an internal option), the same for the assertion with hasAttrPath
<vcunat>
hmm, mergify seems intriguing for this, at least at superficial look
<infinisil>
mergify?
<srhb>
infinisil: (for auto merging prs)
<srhb>
infinisil: Yeah, agreed. I know johanot wants to combine this into his rather massive effort for k8s in 18.09 so I don't want to nitpick too much and hold things back.
<srhb>
infinisil: But I always get nervous when it becomes sorta-easy to accidentally import secrets.
<infinisil>
srhb: How can you accidentally import secrets with this?
<infinisil>
Or with slight carelessness on changes
<clever>
infinisil: src = ./.; in the wrong dir
<srhb>
infinisil: I'm mostly scared of the if isAttrs v then (safe decl stuff) else (potentially unsafe imp stuff with v :: path)
<infinisil>
clever: What's this got to do with the PR though?
<srhb>
It might not be as risky as I feel, but it's a bit complicated juggling.
<clever>
infinisil: hadnt read the PR yet
__Sander__ has quit [Quit: Konversation terminated!]
<infinisil>
Hmm..
<srhb>
infinisil: It's a nit, but I'd prefer if the path v travels through in the impSpec case is a very clean (toString v) at the earliest possible time. :P
<srhb>
But yeah, no biggie. Just a minor concern
<gchristensen>
I am also a big fan of opting for simpler than more complex in the module system
<infinisil>
srhb: Something like using `apply = v: pkgs.writeText name (builtins.toJSON v)` on the spec option?
<infinisil>
In the case of a path only I guess..
<infinisil>
But that would complicate it some more
<infinisil>
It just doesn't feel right to have 2 options for a mapping from name -> path and name -> attrset, where the attrset is always convertible to the path
<infinisil>
But if more people like that then I won't hold against it
pie_ has quit [Ping timeout: 244 seconds]
<srhb>
infinisil: I don't want to step on any toes, you've been putting a lot of work into those reviews. I don't feel strongly about it.
<infinisil>
srhb: Posted a comment about it :)
vcunat has quit [Quit: Leaving.]
<srhb>
infinisil: Well summarized, thank you!
Ericson2314 has joined #nixos-dev
<infinisil>
Huh what did I miss, hydra didn't evaluate in 3 days and all logs are wrongly encoded
<srhb>
infinisil: logs should be okay going forwards, evals are.. Not good
ma27 has joined #nixos-dev
ma27 has quit [Client Quit]
ma27 has joined #nixos-dev
pie_ has joined #nixos-dev
<infinisil>
gchristensen: You can update the release managers in the topic btw :)
<infinisil>
samueldr: Regarding commits not going through PR's, I'm currently fixing a bunch of packages without PR's, which didn't even build before, you think this is okay?
<gchristensen>
I think it is probably still preferrable to do a PR, but maybe on the lower end of "hmm wish that was a PR"
<Ericson2314>
infinisil: you can always do a PR and then immediately merg it
<infinisil>
Alright, will stop that then
<samueldr>
infinisil: depends on the semantics, but yeah PR+merge after eval/build from ofborg (maybe not even build?)
<samueldr>
but if you fix "the same issue" in multiple packages, it's (obviously?) one PR, right?
<infinisil>
Yea sure
<gchristensen>
it sucks having loads of PRs fail eval checks, is why I advocate for the PR strategy :)
<samueldr>
does github have an equivalent to gitlab's "merge when checks all succeed"? I guess not since there are mergify services :/
<infinisil>
gchristensen: Well I of course build all my changes before pushing, but yeah, I get what you mean
<LnL>
I think release.nix explicitly doesn't recurse either IIRC
Lisanna has joined #nixos-dev
phreedom_ has quit [Ping timeout: 250 seconds]
orivej has joined #nixos-dev
phreedom has joined #nixos-dev
ma27 has quit [Quit: WeeChat 2.1]
jtojnar has quit [Ping timeout: 260 seconds]
jtojnar has joined #nixos-dev
ma27 has joined #nixos-dev
ma27 has quit [Client Quit]
ma27 has joined #nixos-dev
ma27 has quit [Client Quit]
ma27 has joined #nixos-dev
ma27 has quit [Client Quit]
ma27 has joined #nixos-dev
ma27 has quit [Client Quit]
<samueldr>
I'm uh, probably a bit too shy :/ what's the expected process for sharing fixes on PRs, I kind of feel it's rude to "just push on their branch", and also kinda feel bad just closing and making a new PR with their changes + mine on top
<samueldr>
(and I'm probably overthinking this!)
init_6 has joined #nixos-dev
<samueldr>
s/shy/polite/
ma27 has joined #nixos-dev
ma27 has quit [Client Quit]
ma27 has joined #nixos-dev
init_6 has quit []
<gchristensen>
sometimes I'll make a branch off theirs and send them a link to the diff of what I added
<gchristensen>
and say "I could push this to your branch and merge it as-is if it looks good to you"
<samueldr>
thanks for the input, I was about to say something like that
<samueldr>
it's awfully convenient how github allows you to refer to commit IDs on any fork repos
<gchristensen>
whoa
<samueldr>
(and through th PR API, you can get an already-merged commit ID for the PR)
<samueldr>
(but I guess that's something you already know from ofborg)