Replies: 15 comments 3 replies
|
I agree with this approach, but I must also emphasize ability/expertise. And on the other hand, 4 or 5 people with various schedules and availability will be a bottleneck for sure. Also, once the project (hopefully) reaches HA Core, what happens to this structure? A couple of people would remain code-owners of the "myhome" domain in HA, but OWNd would remain in its own repo (here), right? |
|
Works for me as written, 72 hours and "silence is not approval" included. Two small additions, then Julien's questions. First, the one approval that branch protection asks for should come from whoever knows that part of the code, so maybe each of us says where we feel at home. For me that's the covers: timing, calibration, what the bus really does. Second: what I did today (fixtures out, the four branches protected) I did under the "urgent privacy" clause before it existed; happy to adjust any of it here. When it comes to reviewers: I agree, a wider circle of collaborators with review rights and no admin rights would help a lot, and a few people in here have been reading the code carefully, without anyone asking them to. On Core and on pace: I understand the temptation, after months of a quiet repo, to just let the one person who is moving fast keep moving. I'd resist it, and not for reasons of process. This integration switches lights, drives shutter motors and sets heating in people's homes. A change that looks fine and isn't can leave a shutter running against its end stop, or a boiler on; that is a different kind of "we'll fix it later" than a broken card. So my preference is small, well-explained PRs that one of us has actually read before they land, even if that means the branch moves slower for a while. Core reviewers will ask for exactly that anyway... so slowing down now is also the shortest road there IMHO. |
|
I understand the concern, and I agree that changes affecting physical devices deserve careful review. However, I think we should distinguish between knowing a particular part of the codebase deeply and having more hands-on experience with certain features or devices. Both are valuable, but they are not necessarily the same thing, and assigning review responsibilities purely by feature area may be harder than it sounds. AI-assisted development and review are also part of how many of us work. That does not invalidate anyone’s expertise, but using those tools—whether to write a change or review it—does not by itself establish that the result has been understood and verified. We should be clear about what each person has actually checked: the implementation, the protocol assumptions, or the behaviour on real hardware. My concern is that we are approaching a standstill. I believe we need to move as quickly as reasonably possible towards a solid, clearly scoped 2.0 baseline, with known limitations documented and critical issues addressed. We should not wait for v2 to be perfect in every area before releasing it. Broader testing on real installations will help us discover combinations and behaviours that our own setups cannot cover. From that baseline, we can fix reported problems and add further functionality incrementally. I support meaningful review. I would also like us to agree on what genuinely blocks the initial 2.0 release, what can follow afterwards, and who is available to review the remaining changes. That would give us a practical path forward while respecting everyone’s time. |
|
That's a fair point on "what each person has actually checked", and it applies to me first, so let me be open about how I work. I can't read code at this level line by line. What I can do is compare a change with the same thing implemented on my fork, which has been running on my twelve shutters for a week, and with what I measured on the bus with a tape measure; Claude does the line-by-line comparison for me. The result is a short list of concrete findings, each with a file and a line, that anyone can check in a few minutes. The review I'm posting on #318 is an example. Some of those points may turn out to be wrong, and I'd genuinely like to know when they are; the ones that hold would otherwise have reached people's shutters. That's the contribution I can make, no more than that. On the review rule "already blocking progress": it has been in place for a few hours, one PR is waiting on two small change requests, and five others were merged in the meantime, so I'd give it a little time before drawing conclusions. Where I fully agree with you: let's write down what blocks a 2.0 beta and what doesn't, so that nobody feels stuck behind a vague veto. My list would be short: the privacy fixtures (#316), the admin check on the raw bus command, and core 2026.9 compatibility. Everything else can flow as small PRs behind it. I'll look at the cover-related ones as they come, in my spare time and in the order of the stack; if someone else gets to one first, all the better. About a wider circle of reviewers: I agree with both of you, it's the only realistic way to keep the rule without it becoming a bottleneck, and a few people in these threads have already shown they read the code carefully. |
|
Proposal: CI is the gate, review is risk-based, beta is where we learn We are not building a rocket; we are building a bridge to a fragmented bus (F454, MH200N, MHS1, F422 routers, DALI, firmware quirks). However much we read, the code will contain bugs that only a real plant can find. So the process must be built to find bugs fast and undo them fast, not to keep them out by review alone. 1. Two branches, two rules
2. Review by risk, not by size
3. What "good CI" has to mean to earn this 4. Beta cadence 5. Review etiquette — this one is not optional 6. Decisions The concrete state, so this is not abstract: the 12 open PRs are being re-stacked into 9 linear ones (fixtures+CI → issue batch → cover model → card+calibration → quality scale → naming → typing → skeleton → router), merged only ever from the bottom. Interstellar0verdrive's five points on #318 are real Class-A findings; they get one pass from me, and then — per point 5 — a re-review from the person who asked. Meanwhile, at the top of the rebuilt stack: 1473 passed, but two leftovers from the reorder — 10 uncovered lines in |
|
Thank you both. I think we are getting closer to a workable agreement. @Interstellar0verdrive, I appreciate the explanation of what you actually verify and how. Your hardware experience and concrete findings are valuable. Fair point also that the review rule has only been in place for a few hours: my concern is about it becoming a bottleneck, rather than saying that this has already been conclusively demonstrated. @GreenGrassBlueOcean, I support separating the beta integration branch from stable and using real-world feedback to improve an initial 2.0 baseline. I would start by agreeing on the short list of beta blockers: privacy, admin protection for raw bus commands, and compatibility with the current supported Core. Any other proposed blocker should identify a concrete problem and explain why it must be resolved before beta. Features and improvements that are not essential to that baseline can follow incrementally. On reviews, I agree that follow-up matters, but we should allow another available reviewer to take over when the original reviewer cannot respond. A known unresolved defect should not automatically become acceptable after 72 hours. Equally, once the concern has been addressed and verified, the PR should not depend indefinitely on one specific person's availability. I would also classify refactors by their actual impact: changes to routing or lifecycle can affect actuation even with 100% coverage. My preference is to agree on a simple process for this beta, use it, and adjust it based on experience. We need a solid starting point that we can test more widely and improve, without making every future enhancement a prerequisite for releasing 2.0. |
|
I'd sign @xtimmy86x 's version as it stands: the short list of beta blockers (privacy, the admin check on raw bus commands, current Core compatibility) as the starting point, and "another reviewer can take over" as the answer to availability. Two things in the counter-proposal I can't get behind, said once and briefly:
On "Interstellar0verdrive owns covers": glad to look at cover PRs when I can, and I'd rather not be the only gate on them . Weekly betas on an opt-in channel with a documented rollback sound right to me, once the blockers are in. |
|
Thanks, this works for me too. My concern was always about availability becoming a bottleneck, and allowing another reviewer to take over addresses that without treating silence as approval. I’m comfortable keeping one independent approval on both branches, with the depth of review proportional to the actual impact of the change. Areas of experience should help us find a reviewer, without making any one person the mandatory gatekeeper. The example from #318 is a concrete reminder that green tests depend on how accurately they represent the real library and hardware behaviour. That is useful feedback to turn into a regression test. I also understand that nobody can guarantee a fixed response time when contributing in their spare time. If someone cannot follow up, making that clear in the PR lets another maintainer pick it up. From here, I would focus on agreeing on the short beta-blocker list, resolving those items, and getting a clearly scoped 2.0 beta into wider testing. Further features and architectural improvements can follow incrementally, without continually expanding what must be finished before that release. |
|
Hi everyone, I’d like to take a step back from the specific PR mechanics and look at the bigger picture behind the sentiment raised over #318: "All tests were green, but the mock lied and the code had a bug—therefore we must slow down and rely on deeper manual reviews before anything merges." First off: @Interstellar0verdrive’s catch on #318 regarding the To explain why, let’s start with an analogy. The 4 Golf OwnersImagine the four of us all own a Volkswagen Golf 1.9 TDI. Three of us have fabric seats, one has leather seats, and the paint colors are different, but mechanically, they are the exact same car. We get fed up with the factory ECU software and decide to build an open-source vehicle control system. We proudly proclaim on our repo: "The best control software for ALL Volkswagen Golfs." We are rigorous engineers. We write clean code, maintain strict typing, demand 100% test coverage, and institute mandatory peer reviews. We review every pull request line by line and test it thoroughly on our own 1.9 TDIs. On our driveways, the cars purr like kittens. Everything is green, every review approved. Then we release it to the world. A week later:
The Reality of OpenWebNetIn the MyHOME world, we are those 4 Golf diesel owners. Between us, we have a tiny sliver of the actual hardware spectrum:
Meanwhile, the OpenWebNet universe out there is vast and fragmented:
No amount of manual code review by 4 people can mentally simulate how a 2008 F452 firmware behind an F422 router will behave. In #318, the mock didn't lie because someone was careless; it lied because neither the author nor the test harness had telemetry on how The Danger of Process ParalysisIf we convince ourselves that safety comes from heavy manual review gates and waiting days for approvals on every change:
The True Safety Net: The Community Trace FlywheelWhy did we build Because real hardware telemetry and bus traces are the only way to map the unknown universe. If we run fast, weekly opt-in betas from our integration branch:
Once a real bus trace becomes a CI fixture:
That was my primary motivation for building the trace and sweep infrastructure. It would be a huge pity if we end up with immaculate, heavily reviewed code that only runs on our own 4 setups, while drying up the pipeline of real community traces because releases take months to reach anyone. What I Propose
Let's embrace community bug reports and bus sweeps as the fastest, safest way to build an integration that truly works for the entire OpenWebNet ecosystem. |
|
I agree with the value of frequent opt-in betas and turning community findings into regression tests. That is also why I want us to release a clearly scoped 2.0 baseline without waiting for every planned feature or quality improvement. I think we already share that goal. The proposal to keep one independent approval also includes a short beta-blocker list, interchangeable reviewers and proportionate reviews. It does not require us to validate every possible installation before releasing a beta. The #318 example shows how these approaches can complement each other: a review identified a gap in the tests, and that finding can now improve the automated checks. Community testing will expose other gaps that none of us could identify on our own. Trace replay is valuable, but we should also be clear about what it reproduces. Parsing recorded frames is different from reproducing socket lifecycle, timing and physical behaviour; some findings will need dependency-level or timing tests alongside the trace. Could we settle the remaining concrete question: are we keeping one independent approval on the integration branch, with any suitably informed reviewer able to take over? That is the approach I support, together with prompt handover when someone is unavailable. Then we can focus on closing the agreed beta blockers and publishing the next beta. I would like us to put that practical agreement into GOVERNANCE.md and improve it through experience, while keeping development moving. |
|
Candid question: what are we trying to catch with systematic manual review? Bugs before they can manifest in the wild? Exotic edge-cases? I agree that a better approach would be to clearly state: you're trying a BETA, from a project that has had a big development shift, there WILL be bugs (mostly setup specific ones), we need your help reporting them, here's how, and then here's how to roll-back to the last version that worked for you. In that case, it might be better to do a periodic deeper dive to audit all or some aspects of the code and MAYBE catch something that went unnoticed the same way it MIGHT have been caught during a regular review. Overall, I agree that given the few people able and willing to take this project in hands, it might be a long term net negative to discourage. I understand that this disappearance over the week-end with no contact did make people tense up, and raise trust questions. |
|
I agree with @anotherjulien . This is the concern I raised from the beginning: I feel the restrictions introduced by @Interstellar0verdrive are disproportionate to the project's current stage and the number of people actively available to maintain it. I appreciate the concrete issues that reviews have caught, including on #318. Those contributions are valuable. However, I do not think approval from another maintainer should be mandatory for merging every PR. I was willing to support one independent approval with interchangeable reviewers as a compromise, but that still depends on someone being available. Allowing another reviewer to take over does not solve the problem when nobody has the time. My preference is that maintainers can merge their own PRs when the required checks pass, the changes have been appropriately validated, and there are no known unresolved blocking issues. Passing CI alone does not prove correctness, and concrete problems raised during review must still be addressed. For changes with significant impact, we should actively seek additional review and hardware testing. For the beta branch, I favour frequent opt-in releases with clear limitations, straightforward reporting instructions and a documented rollback path. Stable releases should require stronger validation and evidence from beta testing. On trust, I agree that any concerns should be discussed directly. We all want a reliable integration and a good user experience, through a way of working that people can sustain in their spare time. Let's agree on the essential beta blockers and move towards wider testing, with a process based on maintainer responsibility, evidence and collaboration. |
|
Read all three, and I'll take the point rather than dig in: with four people working in their spare time, one mandatory approval on the integration branch is more than the project can carry right now, and I'd rather relax a rule I set than be the reason work sits. So for Two things I'd ask to write into GOVERNANCE.md next to that, because they are what the rule was actually protecting:
On cover changes I'll keep posting what I find, as notes rather than as a gate. |
|
Following up on the "small, well-explained PRs" point, and on Interstellar0verdrive dropping the required approval on the integration branch today: I've split the open work into PRs that each do one thing and can be read in minutes, and put them in one line so they merge from the bottom without anyone having to untangle a stack. Here is the order I'd suggest, and why. 0. Independent, tiny, unblocks a reporter
1. The agreed beta blockers (privacy, admin check on raw bus commands, current Core — the third one landed as #334)
2. One issue per PR — each one small enough to check against the issue it closes
3. Docs and CI, then the gateway
4. Covers (Class A — Interstellar0verdrive's ground)
5. The feature/quality stack on top of #346
Independent, external
Umbrella
How to merge it: always the lowest open PR first, "delete branch" on merge — GitHub then retargets the next PR to the branch it merged into, so the chain never breaks. Every PR is green on its own CI (100 % coverage, privacy check, node suite) and can be reverted alone. Where a second pair of eyes helps most: #346 and #349 (Interstellar0verdrive), #319 and #343 (xtimmy86x). The rest is routine: fixes, tests, docs — CI plus a quick look. If the order looks wrong to anyone, say so here and I'll reshuffle; the split is cheap to rearrange now, expensive after the first merge. Edited 15 Sep: step 5 updated after the #319 split — calibration is now #349. |
Uh oh!
There was an error while loading. Please reload this page.
Hi everyone! Now that we have a shared maintainer team, I’d suggest agreeing on a simple way to make and record decisions, so they don’t get lost in this growing thread.
My proposal:
For proposals, a short explanation of the problem, suggested solution and impact should be enough. Let’s normally allow at least 72 hours for feedback, and longer for major decisions, while allowing people to explicitly abstain when they don’t have the time or expertise. Silence shouldn’t count as approval.
Once we agree, we can update the opening post with the decision, its reasoning and links to the implementation issues or PRs. If we can’t reach agreement, we should clarify the remaining concerns before proceeding and agree on a fallback voting rule for organisational decisions.
If this approach works for everyone, we could create a “Proposals & Decisions” category and capture the agreed process in a short GOVERNANCE.md.
@anotherjulien @GreenGrassBlueOcean @Interstellar0verdrive @fedem95 — what do you think? I’d like to keep this lightweight and practical for everyone.
All reactions