search

artsy / README

:wave: - The documentation for being an Artsy Engineer
Creative Commons Attribution 4.0 International
1.1k stars 120 forks source link

RFC: Repos should contain HACKS.md document #390

Closed damassi closed 3 years ago

damassi commented 3 years ago

Proposal:

Repos should contain a HACKS.md document that details hack-like changes to a codebase.

Examples include:

See Eigen's HACKS.md file for a healthy example of what can live within and how entries could ideally be formatted.

Reasoning

This was spawned by Eigen's good example and @pvinis's initial idea, but the gist of it is: long-lived repos often contain quick-fixes and other changes that don't live within normal chains of command. These fixes are often made in the moment and are highly dependent on developer context, which is often lost when staff moves on.

Patches created via patch-package are a great example of a hack:

While a dev can trace their creation back to a PR this often doesn't provide enough context to understand why a patch was created in the first place. And because of these patch changes, library upgrades can become dramatically more complicated ("why was this library change made in the first place? Can it be safely removed?"). By documenting things like this in a HACKS.md file, context is preserved.

This RFC is mostly pointing at repos that already use patch-package (most of our JS repos) but can be extended to any codebase.

Exceptions:

None.

Additional Context:

This RFC was prompted by a discussion in slack around updating MP schema stitching to latest, with the added caveat that there was a non-trivial patch involved. Undocumented patches like this bring fear to every lib update -- especially critical infrastructural ones like schema stitching.

How is this RFC resolved?

A HACKS.md file is added to our common repos and awareness of this new pattern is spread about the team.

Resolution

Decided to try this out.

Level of Support

2: Positive feedback.

Next Steps

Ping primary repo owners with link to thread and template and ask to open PR with new file.

Exceptions

None

jonallured commented 3 years ago

Interesting! I'm wondering about the file name though - why not just call it what it is, something like patch-package-reasons.md or something, haha. Are there examples of items in the HACKS.md file that are not patch package-related?

damassi commented 3 years ago

Are there examples of items in the HACKS.md file that are not patch package-related?

Nope, no other examples, but I'm certain there are too many to count! The only reason we even know about patch package hacks is because it outputs a git patch file. Having a pattern like this in place is meant to centralize this category of technical decision making; there are many forms actual hacks can take.

mdole commented 3 years ago

I love this idea! Two things I'd like to suggest/clarify:

damassi commented 3 years ago

I listed the Eigen example as a way of showing the wide variety of things that can be in there, but will update the RFC to make clearer 👍

I could see it getting very big

Eigen's has been in place for a while now and not too big. I would favor a keep-it-simple light bullet point list for now, as Eigen has done.

pvinis commented 3 years ago

I'm so happy my suggestion inspired an rfc 😍.

I'll offer my ideas behind that file, and of course, do with them what you want :).

if you check the raw file here https://raw.githubusercontent.com/artsy/eigen/master/HACKS.md there is a comment on top that doesn't render on gh.

the goal with this file is to make it empty, right? but probably it will never be. we had and have things like a resolution in package.json of a dep of a dep hardcore's. that's cool, but after a year or so, do we need it? what should we check if it breaks when we remove it? when is it a good time to remove it?

so this file and comment is like: tell the rest of the team, what you did, when can we remove it, and why you did it. that way, every now and then we can check the file and try to remove things.

if you see the eigen file it had things like "we can remove once we remove enzyme" or "when that PR is merged" or "now". there are also not only patch-package, but things like EchoNew which was a Jacky but necessary way to fix a problem.

so yea, that was the idea behind it. basically what Chris says, I'm just adding a bit more background behind its introduction to eigen. :)

MounirDhahri commented 3 years ago

This is a good suggestion 👍. The HACK.md file was useful for us in many situations where adding a comment to the patched portion of the code wasn't enough. The only issue we had with the file is that we forget about it sometimes until someone mentions it during the review, and I guess we can solve that by adding a Danger rule later that checks for new patched files.

kajatiger commented 3 years ago

I like the idea. I experienced once this "why does this code not do what I expect" problem and only later found some patch for a ruby gem hidden somewhere in the code and learned from a colleague that he had patched that library some time ago to make it serve our needs. Having a documentation for that would have saved me a lot of time and confusion. Especially if that colleague had left the company. In general I often think library patching is not a good pattern though. I would rather have people contributing to that library and extend it or just find another workaround. But if you patch, better document it for sure!

dblandin commented 3 years ago

Love this! Can already think of a few good entries for artsy/gravity 👍🏼

erikdstock commented 3 years ago

I like the idea but find the eigen file confusing. For some it looks like there is a future fix for a current hack being described, but it's not explained what the hack is. For others it seems like a troubleshooting guide. Maybe we could have more standard fields like what/why/how/future resolution, and a separate section if necessary for troubleshooting?

damassi commented 3 years ago

@kajatiger

In general I often think library patching is not a good pattern though. I would rather have people contributing to that library and extend it or just find another workaround. But if you patch, better document it for sure!

Totally agree. Whenever I see a patch being applied in a PR I try to guide the dev towards at least proposing the change upstream, and even better opening a PR. As a team, even with this HACKS.md doc, we should continue this, and always keep an eye out for places that OSS contributions can be made in contrast to quick fixes.

@erikdstock

Maybe we could have more standard fields

This is a good idea. Let me think about it a bit as this is the second comment around form. Form is good!

damassi commented 3 years ago

To take this to completion, I think for now we can keep things simple and more free-form, as Eigen does it. If we start adding many hacks and need to formalize some more, I suspect that might be indicative of a larger problem!

damassi commented 3 years ago

Copying the new formatting in Eigen, I've added a HACKS.md file to force id https://github.com/artsy/force/pull/7767 and updated README.md with a new Hacks meta section. If anyone has any feedback please respond over there!

damassi commented 3 years ago

Alright closing this with a status of general support 👍

Follow-up steps:

dblandin commented 3 years ago

@damassi When you have a moment, it might be nice to update this thread with our RFC resolution template.

damassi commented 3 years ago

Updated the main description body 👍