hotfix wrong manufacturer count vs model count

[bluemoehre]

Master is broken since #7961 , caused by commit 9da501cb1e6b4b2b74248837f29212d7ac55d69e, which was not passing / running DDF checks.

[github-actions]

Hey @bluemoehre, thanks for your pull request!

Tip

Modified bundles can be downloaded here.

DDB changes

Modified

• philips/light_zb3_C.json : Hue white and color ambiance light ✔️

Validation

Tip

Everything is fine !

_{🕛 Updated for commit c35d0dd}

[ebaauw]

See also Discord, #general channel.

I think what happened is: Both #7961 and #8001 updated light_zb3_C.json, adding a new model to the file (twice each, for Philips and for Signify). When the #7961 was merged, the merge logic saw that the added entries to the manufacturername array had already been added by #8001, and didn't include the added entries from #7961.

Individually, each PR passed validation, but combined they don't. I'm not sure if validation of open PRs gets re-triggered after another PR is merged, or whether that's requires actively rebasing a PR before merging. Maybe this could have been prevented by a more rigid merging process.

I would feel more comfortable changing the DDF structure into an array of manufacturername/modelid pairs, rather than two separate arrays. That probably would still have caused the same issue, but would make it easier to detect visually. On the longer term, I want the Hue light DDFs to match on Image Type (cap/otau/image_type) and Device Type (type) rather than on modelid, so we no longer need to whitelist each individual modelid.

[github-actions]

This pull request is now merged. The new DDB files have been uploaded to the store.

DDB Files

Modified

• philips/light_zb3_C.json : Hue white and color ambiance light : with hash (01884b520a)

_{🕣 Updated for commit 5b3162b}

[bluemoehre]

I think what happened is: Both #7961 and #8001 updated light_zb3_C.json, adding a new model to the file (twice each, for Philips and for Signify). When the #7961 was merged, the merge logic saw that the added entries to the manufacturername array had already been added by #8001, and didn't include the added entries from #7961.

Individually, each PR passed validation, but combined they don't.

Are you sure about that? When I check the single commit I am referring to, nothing has been added to the manufacturers property. Or does this commit show the status after the merge? Maybe it was a conflict where "one version" was accidentally chosen instead of both?
I don't want to play any blame game here - these things can happen - but I really want to know if we need to worry about the order of merges, or if the tests should better check every commit on master branch. We shouldn't be able to break the master so easily without noticing.

I would feel more comfortable changing the DDF structure into an array of manufacturername/modelid pairs [...]

I absolutely agree about changing that structure! Maybe we should start a feature branch and discuss some examples what matches best the current needs. (This will be also a perfect chance to setup a JSON schema file 🙂 )

[ebaauw]

Are you sure about that?

No. That's why I said: "I think". I do know for a fact that #7961 passed validation and added the two lines to the manufacturername array. The commit introducing the issue is a merge commit, which is only created when merging the PR. I suspect that #7961 wasn't rebased after #8001 had been merged, as that would have triggered the validation.

I really want to know if we need to worry about the order of merges

I suspect the exact same thing would have happened if the two PRs had been merged the other way round.

if the tests should better check every commit on master branch

That would detect the issue but not prevent it. Actually, as per your comment, it would seem the validation did run (and failed) after the merge commit...
I think it might be better to make sure to rebase each open PR with the master branch (merge the master branch into the PR fork) and check the validation results right before merging the open PR.

[bluemoehre]

That would detect the issue but not prevent it.

To detect a broken master branch would be a good minimum requirement, because all other branches which forked or did a merge / rebase in the meanwhile suddenly failed the tests. Maybe it's possible to change the setup to test all single commits on master? This way we could see where it accidentally broke.

I think it might be better to make sure to rebase each open PR

If I remember correctly, this can be achieved by the project rules in Github.