Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [che-dev] Development quality and transparency



On Wed, Nov 28, 2018 at 9:57 AM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:
Docs BTW is also a good example. And if we think in general about the completed feature  - it should be the same unseparated piece as unit tests.

About reviewers.
I believe that both contributors and reviewers are all serious responsible people. They all want to make their contribution according to all rules, so nobody will feeling upset, misunderstood or be a source of complaints. The issue is not that we don't have enough reviewers. 
I think one of the reasons for this conversation is that it was not very clear sometimes at what stage we are in this process for each piece of code.
So reviewers understand what is going on, commiters understand what they must, can and should do.

I think we should be perfectly clear about PR decision making pattern.

I like a statement that Committer is an Owner of the PR and responsible for quality of PR.

Formally and technically PR requires at least one Reviewer to be merged. 
It may or may not be enough for particular PR and Committer is the one who decide when to merge and who's opinion is important for him/her to make a merge decision.  
Normally, approval of one or more Maintainer (Code Owner) is needed. 
(There is also escalation case, if PR is blocked, in this case Code Owner's input is mandatory to make a decision [1])
It does not mean that anyone can not self-assign to review any PR. 
It is highly welcomed and appreciated, however, it should be clear that everyone have voice but not everyone have vote (which perfectly fits meritocracy principle).
 
I think this flow is good enough, no additional roles/processes required?


On Wed, Nov 28, 2018 at 9:50 AM Oleksandr Garagatyi <ogaragat@xxxxxxxxxx> wrote:
We would also need to add support of features and testing on both k8s and OS infrastructures - current CI doesn't cover k8s at all.
So, it seems that the list is pretty big and it might be difficult (or next to impossible for a rare contributor) to keep all of that in their mind.
We should consider a more formal process with a DoD list for different kind of contributions (poc, beta, prod). This would help us to keep the quality good and not argue whether requests from reviewers are objective or subjective.
yes, exactly.

On Wed, Nov 28, 2018 at 9:13 AM Oleksandr Garagatyi <ogaragat@xxxxxxxxxx> wrote:
I wonder whether lack of docs in a PR is part of the issue. It is common to merge a PR and not contribute any docs or say that someone else should document it. But it is hard to properly document something that was developed by someone else.

What if we do a rotation of a "PR quality verifier" position who would be responsible for ensuring that reviewers checked the presence of docs/unit tests/functional tests/CI build and tests passing? A week for everyone who would signup for that position. This person would be allowed to block PR merge until criteria are met or reviewers negotiated any other way forward.

On Tue, Nov 27, 2018 at 11:31 PM Gennady Azarenkov <gazarenk@xxxxxxxxxx> wrote:


On Tue, Nov 27, 2018 at 11:16 PM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:


On Tue, Nov 27, 2018 at 10:35 PM Brad Micklea <bmicklea@xxxxxxxxxx> wrote:
On Tue, Nov 27, 2018 at 3:24 PM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:
Thank you Mario. Some comments inline.

On Tue, Nov 27, 2018 at 6:46 PM Mario <mario.loriedo@xxxxxxxxx> wrote:
@Sergii Kabashniuk for the sake of "transparency" I am going to provide my (personal) point of view on the issue you are raising:

Lack of Unit Tests
That was already tracked in this issue and I would suggest that you open an issue as this one if you spot some area in the code that are not well covered. That will make it easier to prioritize and involve contributors.

I tend to disagree with the general statement of this paragraph. Yes. "Clearly" declared POC can have an only source code. Code that we are putting in the master of Che repositories must contain "UnitTests" according to its specification as an unseparated piece. The necessity to have a reasonable maximum of unit testing is "code owner" obligation. It is something that should not be prioritized or planed separately. Tests are not a feature. I don't know how to compare a reasonable number of tests with a new feature. For some reason, we don't have this issue with Java Unit test.  My directly raised 
in GitHub comments questions were mainly related to Go-based code.  I don't see a reason (except to get some time to study) why we should have
approximate coverage percent dramatically less than for Java-based code.

Use of branches and forked repos
The repositories/branches you mentions are all temporary. Please let me know if there are some repositories that we do not plan to merge because that's something that need to be fixed!
 
 Unfortunately, I'm not an owner of that repositories and I don't know the amount of work that has to be done to contribute them back to upstream. However, I can volunteer to identify them, create and track the progress in one issue and have full my assistance to do that anyone who needs help.


Reduce quality standards to do something quick for some event
Quality is tracked by our QE team. There hasn't been any particular high rate of regressions in the last period. In the contrary there have been some great improvements. Of course Che 7 workspaces are not covered by QE yet but this is still considered a "tech preview" so that's normal. But  before  and that's the problem we need to  have been doing code reviews for every, we haven't broke 
 
By quality, I meant Unit test first of all. I think one of the traps I've got here is that not clearly understanding what level of requirements to the code should be applied. Is this POC or is this production ready-to-go code? 


Major code developed in github repositories non belonging to eclipse org
Again, this has been used for initial development of "tech preview" features. I don't see any issue with it. We need to get rid of these repositories before releasing Che 7 as beta.

I don't see the problem either if we have a clear understanding of what is it, how to use it, why we using it.
 

Tracking upstream Che related issues outside eclipse/che
I agree with you on that one. We need to be more careful with that. Asking the issue author to move it upstream or adding a comment on the issue may be enough for now.  

Usage of https://github.com/ws-skeleton/ repository
I am going to repeat myself but...it has been used for initial development of "tech preview" features and I don't see any issue with it.

And about your proposals:

Repository with zero number of unit test is not an option
We should definitely avoid merging code without unit tests in master. But 1) we cannot enforce such a rule on forks or branches where preliminary work has just been started 2) code reviewers should be flexible enough and merge a PR even if it's not perfect (i.e. a new contributors PR, a urgent fix etc...) 3) quality is not about unit tests only: there can be awful code covered by awful unit tests and brilliant code with no unit test and I would rather merge the latter.

I tend to agree with a general statement. The trick for me is to how to send a clear message about at which stage we are. We are experimenting, this is POC, we are toooo far away from GA. To make sure that fast POC nobody understands as quick feature release in GA standards. Sometimes it's quite hard to fix forgotten technical debt.


We must move all code to https://github.com/eclipse/che-* repositories
Yes, this is the plan. We need to get that done for Che 7 beta.

Abandon the practice to hide code development in nonEclipse repositories
As long as we track the issues in eclipse/che and cross-reference them in the PRs done in other repositories I am ok with it. And for initial development I don't see why we should avoid using non eclipse repositories if that makes sense for some reasons: we are not sure of what CQs need to be created yet, we have requested CQs but have not approvals, we want to prototype something without breaking the rest etc...

Keep the practice of features-branches
Contributors should be free to do whatever they want. I am against any hard rule on that.

Your other suggestions are kind of duplicata of others so I am not going to comment on them...
My other comments:

1.  As far as I can see one of the reasons to use https://github.com/ws-skeleton/* repositories is our CQ process that is not fast enough to handle our development speed. I see such action items here:
- Think about even more fast CQ process.
- Think about something https://github.com/che-incubator/*

2. idea development process.
idea -> poc -> beta -> production-ready code in master of https://github.com/eclipse/che-* repositories 
I think one of the reasons for this conversation is that it was not very clear to me at what stage we are in this process for each piece of code.
Any thoughts about how to make it fairly simple to avoid misunderstanding and frustration about an unintended complaint?

Would issue tags work for this? A side benefit would be clarity for the person writing release notes about whether a feature was intended as POC, beta or production ready code. That's not always clear for the RN writer either...
For me as a community member if I want to be a part of the process of 
 - problem identification
- part of the decision making
- part of implementation POC
- early reviewer and adopter.
- to use ws-skeleton/* or che-incubator/*  as a playground.

What those repos(?) intended for? What is lifecycle of things inside? Why I (as a community member) should/can/must use it?

Would issue tags be enough? I guess no. At least not for all tasks.
_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev


--

Sergii Kabashniuk

Principal Software Engineer, DevTools 

Red Hat Ukraine

skabashniuk@xxxxxxxxxx    

_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev
_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev


--

OLEKSANDR GARAGATYI

SENIOR SOFTWARE ENGINEER

Red Hat 



--

OLEKSANDR GARAGATYI

SENIOR SOFTWARE ENGINEER

Red Hat 

_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev


--

Sergii Kabashniuk

Principal Software Engineer, DevTools 

Red Hat Ukraine

skabashniuk@xxxxxxxxxx    

_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev

Back to the top