We all know that one area we are behind on is reviewing new bindings so they can become merged into the baseline. So my question is what would it take for someone to become a reviewer?
My thought is if we can recruit a few more people who may not have time to write coffee but are willing to review code maybe we can ease the burden. I’ll volunteer.
Maybe a few not so experienced developers or those without a while lot of time can review bindings for some areas and the experts would only need to review the hard parts.
Is it to check for security flaws, malicious code or is it taken to the max and each line of code is looked at to see if a more efficient way could be coded?
There are different characters of code review people.
For example I’m very pragmatic and approve if the new code does not break existing functionality and uses new OH techniques (in contrast to also found older techniques in older bindings) and doesn’t bring us into legal problems (license conformance).
Other maintainers perform a line by line review. That’s also a valid approach.
With the static code checker (SAT) we already perform a lot of automatic checks. SAT can even be made stricter to catch more coding errors (resource freeing and switch case handling for example). That’s what we will do in the future and hopefully automate even more. (We still need to correct people on how to use the logger for example.)
To answer the initial questions: You indeed need a deeper Java and openHAB techniques and patters knowledge as all the low hanging fruits are already checked by software.
That’s part of the question as well. I only know that we have dozens of bindings awaiting review before they can be merged into the baseline. It has been stated by Kai and other maintainers that lack of reviewers is one of the problems getting that backlog worked down. So my ultimate goal is to see if some of us who may have the skills to read code but perhaps not the time to write PRs might be able to contribute and help work down the backlog.
What the bindings need to be reviewed for is indeed a key question to be answered and I should have included it in my OP.
That’s what I feared but didn’t want to assume. If something does come up that needs to be reviewed that cannot be automated please do ask for volunteers. I think this is one place where we can leverage the community a bit.
Binding/thing definitions files (XML files in ESH-INF folder) doesn’t need Java knowledge, but are very important files from review point of view. Many bindings do similar stuff differently, so it would be beneficial to try to harmonise binding and thing definitions between different bindings.
Another important file which could be reviewed by regular users is README.md file which describe how binding could be used. For binding developer, this is normally hardest part as developer itself knows too well how binding is working.
Then of course these volunteers would need to be trained on what the “accepted” way should be so we can identify when a binding deviates from that.
That can be challenging for regular users too as they will have to know the technology and be able to exercise the binding to be able to assess whether the README is sufficient. Users who don’t know the technology won’t know enough to tell if the instructions are sufficient.
Well, volunteer could be advanced openHAB user which have used many other bindings and therefore knows how things are done in other bindings.
Maintainers most probably don’t know the technology either, but regular user might own the devices for the new binding or otherwise know the technology but doesn’t have Java knowhow. So we should encourage to regular users give comments even they don’t know anything about Java or programming in general.
Definitly yes, especially when the binding is of interest for you as user it makes a lot of sense.
Functional testing could go in line with a review of the readme (usually your starting point as functional tester as you need to know how to test …)
Usually there is some time passing by until some of the experienced reviewers take a look and they sometimes don’t even have the hardware. So any help is usually appreciated. As well from a binding developers side as interest in your “code sponsoring” is shown. I really liked to see people testing my binding and even using it ‘productively’ before it had been accepted.
I believe the SAT does this as I was using it wrong until it stated to use {} in the string, it was handy to get feedback like this early on so I did not put loads of lines in that need correcting later on.
@rlkoshak
I understand why you are asking the original question, and I also suspect I know another reason why the bindings are stacking up. This is sadly an opinion by some that bindings should not be created if another way to do something already exists. I have seen (thankfully in the past and less so recently) people get told by maintainers that they should not bother creating a binding as there are already too many backing up for review. Only 12 months ago when I started the IpCamera binding I was asked things like “What problem does this solve?”, “why bother as that is a lot of work when we have other ways?”, now the thread is one of the most actively viewed on this forum so clearly people feel there is value in the binding… Once again it comes back to volunteer work and unless a volunteer reviewer sees the value in a binding, they won’t bother to make time to review it. Bindings stack up and the writer feels less encouraged to help out, then the maintainers complain they are low on helpers.
So what about the idea of moving all bindings towards a marketplace feature and some could still be built in and not need a download, and then you simply tick an option to see non merged bindings that then require a download? This way non reviewed bindings can be found and used easier and also star rated by users to indicate how well they work with negative star ratings to warn you away from the bad…
The second advantage to this is it would make life easier for people that want to use a new binding version to get new features or a bug fix and stay on an older Openhab that is more stable then the current one.
Third advantage is if a bug fix is made to a binding just after a release, it can still reach users without needing to wait until the next milestone or stable build.
Forth advantage is you could use the marketplace to automate displaying breaking changes. When the user wants to press the button to upgrade V1.1 to V1.6 of the binding, they can get a message listing all the breaking changes between those two versions, then get asked to confirm if they wish to upgrade.
Fifth Advantage is it could be a rule that you need to earn a 4 star or higher rating on the marketplace before it can be pushed to the project. This way it is the users that vote for a binding and it shows which bindings are used, valued and working well enough to get accepted into being one of the reviewed binding.
The marketplace already needs to be changed and could be reworked to make a system like this work much better.
Yes I am asking for people willing to test a new build of the ip camera binding and I still have not gotten a single report back from someone that it works or does not for them… It is stable and working well for me and the feature is huge as I can now cast any of my cameras to my TV using the chromecast binding. I only call it an Alpha as I am still making changes and improvements to the readme, the logs to guide users and catch issues and am making improvements that change features.
He is probably referring to the eclipse marketplace. That will be shutdown for openhab though, as we are no eclipse project anymore.
And yes I already suggested a staging repo where we would add non reviewed, or rather less reviewed bindings.
Openhabs architecture (we are running in one single process space in one single java virtual machine) does not allow to enforce isolation though. So we must at least scan for malicious code and epic failures like shutting down the openHAB main thread pool (yes every binding can do that by accident very easily)
I will throw in my 2 cents here and answer the thread topic from another point of view.
“What would it take for someone to become a binding reviewer?”
I think the question is a bit too common.
As we have read in some posts already it will require a bigger effort to become a full code reviewer, with push access to the repository.
But that doesn’t prevent anyone from reviewing Pull Request, even if one is not that familiar with java and the code of that special binding.
When i have enough spare time, i am helping out with readme reviews in the openhab2-addons repo.
Mostly i can’t tell too much about the content, but i am familiar with how we build the docs and can throw in suggestions for the writing style and how the readme is written.
If you have the binding hardware, you can of course test the binding practical and give feedback.
Reviewers will be thankful for your help, since it makes the review easier and may speed up the review process.
It’s already possible to downlad and install bindings from pull requests, but the installation process may be a bit too advanced from what i read here.
Marketplace was a nice solution for this problem, so we should reach out for a valid replacement.
But as david said, a new binding should at least have some rough review for malicous code before getting into a testing phase.
Really good point here folks, reviewing the bindings begins with logging on to git (you need an account) and digging in. There is nothing stopping anyone from making review comments. The developers respond to any help. If you think you know java well enough to begin learning to review bindings, get involved, learn the basics, follow along.
When I wanted to help with the docs, Jerome taught and encouraged me. He (and others) reviewed my changes, suggested improvements, taught me the platform and got me thru the process.
Feel free to copy any of my text and post into a new thread, I don’t mind… I prefer to spend the 1-2 hours I have spare coding and not typing on a forum.
I don’t really see it as my idea as this issue of too many bindings building up in the que has been going on for years and the Marketplace was a solution of sorts put in place. See link below.
David is creating a new UI and I am sure he will have some ideas on how to implement something that is an improvement over what we have now.
Believe it or not but all pieces except two are done to solve this scaling problem and enable us to have a staging repo or alternatively to publish bindings that are added as pull requests.
A new buildsystem that builds up on standard Java OSGi ecosystem
We have one type of repositories now (maven) and no more Eclipse p2. That always complicated stuff with having multiple addon repos.
Any maven plugin can be used. We don’t need to consider complicated tycho lifecycle problems.
We have BOMs now (Maven bill of materials). So a staging addon repo’s buildsystem file is only about 50 lines including automatic upload to bintray, because we can reference those BOMs from other repos.
A better way to handle 3rd party dependencies
I have rewritten the extensionservice part of openHab core to allow users to add maven and karaf repositories and I will propose to add a bintray location where pull request builds are located. It also supports a maintenance-status for extensions (abandoned, beta, release). I like the idea of a rating and comment system. That is not yet included but would also require openhabCould to be installed.
What is currently missing is:
The continuous integration service (travis-ci) must be instructed to
build a new-binding pull request with very strict static code checker settings. A failed build will motivate the developer to fix all the issues that we at the moment need to point out manually.
upload a successful new-binding kar file (karaf feature file) to a staging karaf repository on bintray.
A community review procedure. Personally I think a 5000 lines new binding cannot be reviewed correctly by us maintainers. The community must help with functional testing. Maintainers will only read over and help with choosing the right openHAB pattern and resource usage (that is mostly done wrong unfortunately).
And yes my paper ui design study already supports managing multiple repositories. That will work as soon as OH core can accept my extensionservice changes.
With procedure I mean: A way that you can tell in an automatic or semi automatic fashion that the binding works as intended. So that maintainers only start to perform a complete review (can take several hours) when the binding is actually working and deemed useful by the community.
I imagine that we have a forum section where a bot adds new binding threads for people to discuss them and add an “approve” tag or something which results in the github pull request to be marked as “community approved”.
That’s indeed what I meant in the OP. I’m looking for some way to spread the burden among some of the “power users” will many not have the technical experience in OH and perhaps even in Java to do the full review.
My intent was never to replace the maintainers as having final approval so I done think we would need to give these reviewers any more permissions on the repo beyond commenting. With the “approved by community” tag or the like the maintainers well know they don’t have to look very closely at certain aspects of the binding.
As to the staging repo/alternative repo for bindings, that’s a great idea. The whole ecosystem will benefit if it’s easier for years to find and install bindings that have not yet been reviewed through the UIs.
The only thing I would ask as this is our together is to consider also that we will need a way to distribute Rules Templates as well. It would be nice if we can use the same mechanism.
