Hacker News Reader

Comment by pelasaco | Hacker News Reader

I'm shocked that from all discussion nobody noted the root of all evil:

"It was already late at night (I got carried away). I checked in my refactoring to master and went to bed, proud of how I untangled my colleague’s messy code."

No PR, no code review, no CI. Just a cowboy pushing to the master..

View on HN

> No PR, no code review, no CI. Just a cowboy pushing to the master..

Let's be clear that the dangers here are "no code review" and "no CI".

The others are fine in most circumstances, unless you work on some huge monolithic codebase.

Smaller cross-functional teams should be able to push to master to solve problems then and there without everyone's unanimous approval, and everyone should feel enough safety to be able to make changes to the codebase they share with their team.

The real "root of all evil" is why is someone working against the team from within the team?

This is just how some places work, unfortunately.

We all push to main, no code reviews, no CI etc.

Hell, some people here patch things by downloading the live dll, decompile, update code, recompile and stick it back to live...

You realise that everyone pushing changes to main several times a day is the definition of CI? and it's pretty much impossible to do with code review as a gate for pushing to main. Interesting how CI has become semantically diffused to mean literally the opposite of CI: tooling and processes that delay integration and make it easier to work in isolation.

https://wiki.c2.com/?ContinuousIntegration " The most granular unit of integration should be one step of one refactor. "

CI is more than that, at least in my understanding of the term. You have to have some sort of automated testing before doing the merge. We were doing the same shit (all pushing to the same branch) and it broke constantly before we introduced some tests and merges only after a successful test run on a separate CI server. Just pushing everything into the main branch without any checks is no CI, even if it technically can be called so.

That's `automated CI` which is now or less interchanged with just `CI`, however at face value there is no requirement that all CI be automated, it is just usually better to do so.

For anything non-trivial (read: more than around 10-20k SLOC of C or equivalent; straightforward with minimal branching logic; non-critical) you cannot, in any practical sense, have CI without automation. Unless, of course, you want a garbage system. If you do the minimum things necessary to ensure the system is correct (in both verification and validation senses) and of decent quality, and you have no automation, you will not have CI because you will introduce delays between code change and deployment ranging from days (for smaller projects) to weeks or months (for significant projects). That delay means you do not have CI. And it also encourages batching many changes together so that you only have to have one test run (or a small number of test runs) which is also the opposite of CI.

Now, you could bypass all those tests and reviews and just deploy it anyways. And then you'll have a shitty codebase, but you'll have CI so that's good, right?

I'm not advocating for this from an engineering perspective, just stating that it does meet the semantic root of `CI`, regardless of whether you believe it meets _your_ threshold of "good" CI.

...and getting told by a manager to revert code. The entire environment in the example doesn't sound very healthy

And... "it was already _late at night_" (emphasis mine). I learned a long time ago that not all hours in a day are created equal.

And using clean code wrong. Clean code is not a panacea. If people use it as such and without the correct agile rites, they should do it at their own peril, but blaming it on clean code is at the very least naive (and somewhat dishonest)

Personally, I disagree that DRY is the same as “clean”. Clean code is merely code that has been thoughtfully structured (regardless of repetition) and is relatively transparent as to its purpose.

I've seen a few developers see multiple "duplicated" API formats for instance (for different API's) in the same codebase and think those are "duplicated" and then entangle two completely separate API's together. This is worse IMO

There is accidental duplication to beware of. That is when different things looks the same. Then there is real duplication.. Only the later is bad and should be eliminated. Telling the differences is hard

I'd rather look at a simple arithmaticl expression and know immediately what is going on, rather than have to make up a hard to remember name for every small expression that happens to get repeated one or more times in the name of DRY. The same applies to small bits of non-arithmatical code.

This guy was working at Facebook at the time too.

Seems fine then. Move fast and break things.

Huh, don't you mean Move fast and break society

It was reviewed the next day by his boss.

How is pushing to master not a form of CI?

> It was reviewed the next day by his boss.

Assuming Master is the "protected branch" and not kitchen sink, this sounds like - "we test only in production". Wouldn't or shouldn't reviews happen before merging to master?

I was going to downvote you, but then you made me think - everyone pushing to a shared branch is the most continuous form of continuous integration I suppose.

I worked this way for years before we moved into full branched code review. I would never go back however.

That said, you should still have at least some review and automated testing before your code gets in everyone else's way.

Likely, it was their coworker who saw the changes, then reported the incident to the manager, rather than some sort of manager picking through every commit type process.

Reply on news.ycombinator.com