Technical Debt vs. Precedence vs. Velocity

Technical Debt vs. Precedence vs. Velocity

[Vernon Mauery]

Overall, the openbmc project has done a great job of keeping common 
things in common repositories and machine or OEM specific things in 
separate repositories. But there are a few (because it happens) places 
where technical debt has accrued. I want to support getting this sort of 
stuff fixed so we can have a shiny project, but at what expense? 
Velocity. Reality calls and I was not scoped to fix old code, only to 
write a new feature. I am not the only one running into this issue, so I 
thought it best to start a discussion.

Things like:
1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
2) Use of sdbus instead of sdbusplus
3) OEM code in common libraries
4) Lack of proper error handling
5) Use of insecure functions without proper range checking (memcpy, 
strcpy, etc.)
6) Use of printf/fprintf instead of log<>

I am sure there are many other classes of technical debt that I did not 
list here. But the point is that the code base does have places where 
this sort of stuff exists and has not yet been fixed. What should the 
official stance be? To me, mixing coding styles in a single file is 
worse than having the same wrong coding style in a file. But the best is 
if the whole project matches.

If someone contributes and claims that their newly added technical debt 
is okay because of precedence, we don't really have a way to hold them 
to coming back and fixing the problem.

One way to fix this is to have contributors that want to claim 
precedence go and clean up the file FIRST and then come back with a new 
clean commit. But that does make for a higher bar for entry in some 
cases and possibly slows down velocity.

What say the community members on this?

--Vernon

Re: Technical Debt vs. Precedence vs. Velocity

[Deepak Kodihalli]

On 19/01/18 10:00 pm, Vernon Mauery wrote:
> Overall, the openbmc project has done a great job of keeping common 
> things in common repositories and machine or OEM specific things in 
> separate repositories. But there are a few (because it happens) places 
> where technical debt has accrued. I want to support getting this sort of 
> stuff fixed so we can have a shiny project, but at what expense? 
> Velocity. Reality calls and I was not scoped to fix old code, only to 
> write a new feature. I am not the only one running into this issue, so I 
> thought it best to start a discussion.
> 
> Things like:
> 1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
> 2) Use of sdbus instead of sdbusplus
> 3) OEM code in common libraries
> 4) Lack of proper error handling
> 5) Use of insecure functions without proper range checking (memcpy, 
> strcpy, etc.)
> 6) Use of printf/fprintf instead of log<>
> 
> I am sure there are many other classes of technical debt that I did not 
> list here. But the point is that the code base does have places where 
> this sort of stuff exists and has not yet been fixed. What should the 
> official stance be? To me, mixing coding styles in a single file is 
> worse than having the same wrong coding style in a file. But the best is 
> if the whole project matches.
> 
> If someone contributes and claims that their newly added technical debt 
> is okay because of precedence, we don't really have a way to hold them 
> to coming back and fixing the problem.
> 
> One way to fix this is to have contributors that want to claim 
> precedence go and clean up the file FIRST and then come back with a new 
> clean commit. But that does make for a higher bar for entry in some 
> cases and possibly slows down velocity.
> 
> What say the community members on this?
> 
> --Vernon
> 

I suppose this problem is more pronounced with the ipmi repos, and I 
think the reason is most of that code was written much before the 
current set of coding practices and guidelines have been established.

While in some instances I've posted comments asking the author to rework 
their commits based on the current standards, it's most of the times not 
feasible because of the mixing coding guidelines problem you brought 
out, which I agree is odd to do. At the same time, I'm not sure how apt 
it is to ask a contributor to clean up the file in question first, if 
that's even possible to do at a file level, when their commit is 
intended to solve a specific problem and it does that job.

Sorry I don't have any answers for this right away, the one thing I can 
think of is planned effort to refactor the ipmi code base to meet our 
standards. I kind of feel that might lead to better results than 
cleaning up/refactoring on the go.

Thanks,
Deepak

Re: Technical Debt vs. Precedence vs. Velocity

[Brad Bishop]

> On Jan 19, 2018, at 11:30 AM, Vernon Mauery <vernon.mauery@linux.intel.com> wrote:
> 
> Overall, the openbmc project has done a great job of keeping common things in common repositories and machine or OEM specific things in separate repositories. But there are a few (because it happens) places where technical debt has accrued. I want to support getting this sort of stuff fixed so we can have a shiny project, but at what expense? Velocity. Reality calls and I was not scoped to fix old code, only to write a new feature. I am not the only one running into this issue, so I thought it best to start a discussion.
> 
> Things like:
> 1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
> 2) Use of sdbus instead of sdbusplus
> 3) OEM code in common libraries
> 4) Lack of proper error handling
> 5) Use of insecure functions without proper range checking (memcpy, strcpy, etc.)
> 6) Use of printf/fprintf instead of log<>
> 

Use (your maintainer) discretion, but I think precedence would _rarely_ be
a valid justification for new technical debt.

Our coding style, frameworks, guidelines, etc are all going to change over
time.  The codebase will never be up to date in terms of the current mode
of thinking.  So I would advocate for mixing code that does meet the current
guidelines with code that does not, otherwise it will just get progressively
worse.

> I am sure there are many other classes of technical debt that I did not list here. But the point is that the code base does have places where this sort of stuff exists and has not yet been fixed. What should the official stance be? To me, mixing coding styles in a single file is worse than having the same wrong coding style in a file. But the best is if the whole project matches.
> 
> If someone contributes and claims that their newly added technical debt is okay

Maybe just write down the rules you want, and put them in a README in the
project repos you maintain.  Then you can simply point at this when these
come up in review.  Just make sure your README files get a good review
as well.

I suspect some will want things like this documented at an OpenBMC wide level
and not a per repository basis.  I’m not sure if I like that or not.  Maintaining
is hard, and we all have our different things that make it less hard for us to
maintain.

> because of precedence, we don't really have a way to hold them to coming back and fixing the problem.
> 
> One way to fix this is to have contributors that want to claim precedence go and clean up the file FIRST and then come back with a new clean commit. But that does make for a higher bar for entry in some cases and possibly slows down velocity.

I think this is OK as long as the patch submitter has another option - adding
code that meets the current guidelines without requiring the fix-up.  If they
have a personal issue with mixed styles, they can make the fix-up prior to
their edits.

> 
> What say the community members on this?
> 
> --Vernon

Re: Technical Debt vs. Precedence vs. Velocity

[Tom Joseph]

On Tuesday 23 January 2018 06:58 AM, Brad Bishop wrote:
>> On Jan 19, 2018, at 11:30 AM, Vernon Mauery <vernon.mauery@linux.intel.com> wrote:
>>
>> Overall, the openbmc project has done a great job of keeping common things in common repositories and machine or OEM specific things in separate repositories. But there are a few (because it happens) places where technical debt has accrued. I want to support getting this sort of stuff fixed so we can have a shiny project, but at what expense? Velocity. Reality calls and I was not scoped to fix old code, only to write a new feature. I am not the only one running into this issue, so I thought it best to start a discussion.
>>
>> Things like:
>> 1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
>> 2) Use of sdbus instead of sdbusplus
>> 3) OEM code in common libraries
>> 4) Lack of proper error handling
>> 5) Use of insecure functions without proper range checking (memcpy, strcpy, etc.)
>> 6) Use of printf/fprintf instead of log<>
>>
> Use (your maintainer) discretion, but I think precedence would _rarely_ be
> a valid justification for new technical debt.
>
> Our coding style, frameworks, guidelines, etc are all going to change over
> time.  The codebase will never be up to date in terms of the current mode
> of thinking.  So I would advocate for mixing code that does meet the current
> guidelines with code that does not, otherwise it will just get progressively
> worse.
I agree with Brad's suggestion that mixing code with different style is 
fine. I have come across this debate a number of times in code reviews.
Developers would be put in a dilemma if they have to switch styles with 
code in the current file. Its time to have a consensus on this aspect
at project level.

>
>> I am sure there are many other classes of technical debt that I did not list here. But the point is that the code base does have places where this sort of stuff exists and has not yet been fixed. What should the official stance be? To me, mixing coding styles in a single file is worse than having the same wrong coding style in a file. But the best is if the whole project matches.
>>
>> If someone contributes and claims that their newly added technical debt is okay
> Maybe just write down the rules you want, and put them in a README in the
> project repos you maintain.  Then you can simply point at this when these
> come up in review.  Just make sure your README files get a good review
> as well.
>
> I suspect some will want things like this documented at an OpenBMC wide level
> and not a per repository basis.  I’m not sure if I like that or not.  Maintaining
> is hard, and we all have our different things that make it less hard for us to
> maintain.
>
>> because of precedence, we don't really have a way to hold them to coming back and fixing the problem.
>>
>> One way to fix this is to have contributors that want to claim precedence go and clean up the file FIRST and then come back with a new clean commit. But that does make for a higher bar for entry in some cases and possibly slows down velocity.
> I think this is OK as long as the patch submitter has another option - adding
> code that meets the current guidelines without requiring the fix-up.  If they
> have a personal issue with mixed styles, they can make the fix-up prior to
> their edits.
>
>> What say the community members on this?
>>
>> --Vernon