[DISCUSS] Increase the rules for builds?

[chrisdutz]

Hi all,

unfortunately in the last few months I had partially voluntarily part involuntarily reduced me jumping on everything. However do I think this was bad. I am currently sort of stuck in an endless loop of fixing things all over the place.

Ininitialy, I thought putting up too many rules upfront is bad for the project and could discourage contributors. However is a feeling growing, that if we don't ramp up the rules, that the project will become unmaintainable. More and more core parts of the PLC4J part have become unstable because of the lack of real testing (not excluding myself from blame here). Right now I'm debugging through cascades of problems, that proper testing would have uncovered.

I would like to turn on the coverage rules for at least stuff like SPI, that will fail the build, if stuff isn't tested correctly.
Also would I like to put up a rule (not sure how to enforce it) that we shouldn't commit stuff to develop, if it breaks any of the sub-projects. I know some of you never setup their systems to build C and Go. So I would like to propose, that if you don't build with all profiles enabled, that changes are done in branches and the CI system does the checks for you.

Please tell me what you folks think?

[chrisdutz]

I just had another idea ...

How about we agree on some minimum metrics for the core components (API, SPI, Transports, Cache, ... ) ... metrics like "minumum coverage" and some "sonarcloud metrics" (No criticial issues, no major, not more than 100 trivial, ....)
And then for every driver and language in our "Supported Drivers" page, we display this information per driver and language ... this way our users can get a good feeling on how well each driver is tested. Might also become important with governmental initiatives like those of the EU and the US.

[sruehl]

Sonar is already failing always, sadly this only works when Jenkins builds master. Maybe we can better integrate sonar here that it is being called on PRs and normal commits.

[sruehl]

https://sonarcloud.io/project/overview?id=apache_plc4x

[chrisdutz]

Well if it runs on branches, I doubt Sonarcloud will know how to mix the results ... I think that was the reason why I made it run only on develop. (Or is it currently really only running on "master" ... which we no longer should have)?

[chrisdutz]

And I wasn't referring to enabling sonarcloud, I was talking about enabling the "fail the build if the rules are not met" stuff in the maven plugin.

[glcj]

Hello,
I think it's already been discussed, but wouldn't that way of working make us have eternal branches waiting for the modification in C or Go (or Java)?
I understand the difficulties of partitioning the project in the different languages, but that would avoid the formation of waiting queues.
my grain of sand,
Kind regards,

[chrisdutz]

But we could agree on expedited reviews of these branches. As soon as a PR is ready but needs help before merging, invite others to review. I definitely would have paid attention to that and if I didn't, please excuse me for that.

[chrisdutz]

Is say it won't do harm for new stuff and extensions, however if existing drivers are changed, that's what I think we have to do. Right now we have the situation where in go the s7 and the eip driver no longer are operational.

[hutcheb]

I absolutely agree that something needs to be done to improve the quality of code being committed to the develop branch. We have everything in place to do this with the PR function. This runs the CI process (Which we can add whatever checks we feel appropriate) for each language and if it doesn't build then it shouldn't be merged into develop.

This may also increase participation in the project as picking a random person to review it may be just enough encouragement to get someone involved. I also believe people will be able to learn a lot just by reviewing and commenting on other peoples contributions.

In saying this, I'd probably hold off on enforcing not committing directly to the develop branch, or having a requirement for at least one review. But I think it would be a good starting point to move forward.

On Cesar's concern that it will stall development, I have the same concern. I think it is un-reasonable for people to contribute to other languages if the either aren't interested, don't have the time or just aren't able to because they aren't used to the language. I would be in favor to start using the versioning function of mspec files more, and then tackling versioning issues later on.

[chrisdutz]

Just read your comment ... yeah ... fully agree.
And I wasn't implying to only work on branches and merge to develop if someone else reviews it ... I was aiming for: If your changes in one language break something in another language, don't just push it to develop, but ask for help via PR and review function. But I think with versioned mspecs we can solve a lot of problems.

Ideally we could even eliminate the version again, once all drivers support the same version. Wouldn't like to have dozens of mspec versions flying around ... for all we would also need the test-cases updated/duplicated etc.

[chrisdutz]

The more I think of it, the more I actually like the idea ... Lukasz once proposed using multiple versions of mspecs. Some time ago I added this feature to our tooling. Please have a look at the plc4x protocol and driver ... here you can add a version to the other coordinates.

How about the following approach:

• If you change the mspecs and the build works on all platforms: Simply change the mspec
• If you change the mspecs and the build doesn't work on all platforms: Create a new version of the mspec and update the build to use that in only the parts you are able to adjust

With this we're not blocking anything or rendering other language drivers useless and we can focus on porting changes to other languages as we have time for it.