4 ms·

> I did an inventory of my recent feature flags and realized that about 80% of them aren’t there to roll things out to specific populations, or do any sort of A

by bsima 3y ago

> I did an inventory of my recent feature flags and realized that about 80% of them aren’t there to roll things out to specific populations, or do any sort of A/B testing, but to hide unfinished code. Maybe it’s just me but having unfinished, dead, or scratch code in a production codebase really annoys me. Either finish your work or delete the unneeded code. More than a few times I’ve sunk time out of my day into investigating some code path only to realize it’s completely unused.

deleted 3y ago
[deleted]
erik_seaberg 3y ago
“Unfinished” can mean abandoned, but it can also mean half-baked code that has been pushed to master even though it’s still actively being developed and is nowhere near ready to run. Personally I prefer long-lived feature branches; this is a risk with no payoff. As for experimentation, I like percentage rollouts with segregated control/treatment group metrics. I agree that trivial on/off flags should be replaced by code deployments where possible (at my day job, code deployments happen to be a lot slower).
- hinkley 3y ago
  Companies have an expectation that most of the code they paid for will eventually ship. Pretending like code might not ever be finished is an odd choice. The exception is experimental work. Obviously experiments aren't being merged into master, so they don't need to be included in the arithmetic for any processes that eventually involve master.
- eddd-ddde 3y ago
  Personally, long-lived branches feel like disaster waiting to materialize since by definition they do not integrate with upstream. Yeah, you might be really responsible and rebase and test your feature frequently, but what about other people? They are more likely to break the integration with your code since they can't even see it.
mvdtnz 3y ago
I'd rather have unfinished feature flagged code in master (and therefore production) than have the same unfinished code withering in a long running branch, diverging from master and causing integration problems later. > Either finish your work or delete the unneeded code. It's work in progress. We're working on getting it finished.
- rvrs 3y ago
  >It's work in progress. We're working on getting it finished. So finish it and then I will merge your PR ;) What's the use of putting it in master if it's not finished? It's the author's responsibility to get it merged successfully. If they're taking too long and have to rebase and re-work their code to integrate, that's on them. Pushing it into master is either wasting a reader's time (per the original comment) or, worse, inviting an uninitiated collaborator to use it and cause an incident.
  - hinkley 3y ago
    Somewhere along the way, a new generation of developers thought that 'Continuous Integration' means "automated builds". I wonder what they think that 'integration' word is doing there? Since you're here, I'll ask you. What do you think 'integration' means in this context?
    - rvrs 3y ago
      "Passing CI" is a low bar -- it is only a single layer of defense. Moreover, code has syntactic properties that aren't necessarily evaluated by CI. My comment claimed that there are readability and usability concerns with unfinished code. This is why we have code review. CI is a signal that the code is good to merge, but as a code reviewer, I have the final say. I don't merge unfinished code. If CI passes on your branch but later fails due to lagging behind master, it is on you to get it working before re-requesting review. PS: The HN guidelines clearly state "Be kind. Don't be snarky." (:
      - gizmo686 3y ago
gizmo686 3y ago
Some changes are too big to fit into a effort. A few years ago, I was working on a niche compiler. When the project was first started, the decision was made to inline everything, greatly simplifying the rest of the compiler [0]. This decision had served us well for the better part of its then 17 year lifetime, but was finally starting to cause issues with compile time and memory usage. One of our senior developers, who was intimately familiar with the project, tried on several occasions to allow for things to not get inlined. However, he kept having to give up as it was a low priority background task, and his branch would diverge from the main branch faster than he could keep his up to date. The solution ended up being a rather simple feature flag. Within a few days, he coded a flag that would enable not-inlinging; and updated our test infastructure to look for regressions on unit tests with the flag enabled. Going forward, developers we responsible for making sure their changes didn't cause regression when the flag was enabled; and everyone was able to slowly chip away at everything the feature broke when they had spare cycles. What would have been a major stop-the-world refactor with our most senior engineers, turned into a slow moving non-issue. [0] Lack of turing completeness was and remains an explicit design goal, so recursion was explicitly forbidden.
hinkley 3y ago
We need better visualization than we have, but this is why you attach ticket numbers to all commits. If the code is toggled off for a ticket in progress, great. If it’s toggled off for an epic in progress, okay. If it’s toggled off and the epic is complete/abandoned, then it’s not dark code, it’s dead code. Fire up the chainsaws. The old code behind a toggle should be deleted before the feature is Done. If it isn’t then someone screwed up.

by bsima 3y ago

deleted 3y ago

[deleted]

erik_seaberg 3y ago

“Unfinished” can mean abandoned, but it can also mean half-baked code that has been pushed to master even though it’s still actively being developed and is nowhere near ready to run. Personally I prefer long-lived feature branches; this is a risk with no payoff. As for experimentation, I like percentage rollouts with segregated control/treatment group metrics. I agree that trivial on/off flags should be replaced by code deployments where possible (at my day job, code deployments happen to be a lot slower).

hinkley 3y ago
Companies have an expectation that most of the code they paid for will eventually ship. Pretending like code might not ever be finished is an odd choice. The exception is experimental work. Obviously experiments aren't being merged into master, so they don't need to be included in the arithmetic for any processes that eventually involve master.
eddd-ddde 3y ago
Personally, long-lived branches feel like disaster waiting to materialize since by definition they do not integrate with upstream. Yeah, you might be really responsible and rebase and test your feature frequently, but what about other people? They are more likely to break the integration with your code since they can't even see it.

mvdtnz 3y ago

I'd rather have unfinished feature flagged code in master (and therefore production) than have the same unfinished code withering in a long running branch, diverging from master and causing integration problems later. > Either finish your work or delete the unneeded code. It's work in progress. We're working on getting it finished.

rvrs 3y ago
>It's work in progress. We're working on getting it finished. So finish it and then I will merge your PR ;) What's the use of putting it in master if it's not finished? It's the author's responsibility to get it merged successfully. If they're taking too long and have to rebase and re-work their code to integrate, that's on them. Pushing it into master is either wasting a reader's time (per the original comment) or, worse, inviting an uninitiated collaborator to use it and cause an incident.
- hinkley 3y ago
  Somewhere along the way, a new generation of developers thought that 'Continuous Integration' means "automated builds". I wonder what they think that 'integration' word is doing there? Since you're here, I'll ask you. What do you think 'integration' means in this context?
  - rvrs 3y ago
    "Passing CI" is a low bar -- it is only a single layer of defense. Moreover, code has syntactic properties that aren't necessarily evaluated by CI. My comment claimed that there are readability and usability concerns with unfinished code. This is why we have code review. CI is a signal that the code is good to merge, but as a code reviewer, I have the final say. I don't merge unfinished code. If CI passes on your branch but later fails due to lagging behind master, it is on you to get it working before re-requesting review. PS: The HN guidelines clearly state "Be kind. Don't be snarky." (:
    - gizmo686 3y ago

gizmo686 3y ago

Some changes are too big to fit into a effort. A few years ago, I was working on a niche compiler. When the project was first started, the decision was made to inline everything, greatly simplifying the rest of the compiler [0]. This decision had served us well for the better part of its then 17 year lifetime, but was finally starting to cause issues with compile time and memory usage. One of our senior developers, who was intimately familiar with the project, tried on several occasions to allow for things to not get inlined. However, he kept having to give up as it was a low priority background task, and his branch would diverge from the main branch faster than he could keep his up to date. The solution ended up being a rather simple feature flag. Within a few days, he coded a flag that would enable not-inlinging; and updated our test infastructure to look for regressions on unit tests with the flag enabled. Going forward, developers we responsible for making sure their changes didn't cause regression when the flag was enabled; and everyone was able to slowly chip away at everything the feature broke when they had spare cycles. What would have been a major stop-the-world refactor with our most senior engineers, turned into a slow moving non-issue. [0] Lack of turing completeness was and remains an explicit design goal, so recursion was explicitly forbidden.

hinkley 3y ago

We need better visualization than we have, but this is why you attach ticket numbers to all commits. If the code is toggled off for a ticket in progress, great. If it’s toggled off for an epic in progress, okay. If it’s toggled off and the epic is complete/abandoned, then it’s not dark code, it’s dead code. Fire up the chainsaws. The old code behind a toggle should be deleted before the feature is Done. If it isn’t then someone screwed up.