ADR-0003: Switching to long-lived forks to manage actively maintained third-party dependencies#31079
Conversation
… third-party dependencies
iansltx
requested review from
georgekarrv,
getvictor,
gillespi314,
lukeheath and
sgress454
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| (e.g. Fleet server or fleetd). | ||
|
|
||
| Changes to the dependency must be pushed to a branch distinct from the upstream repo's branches, allowing our fork to be | ||
| easily synced with upstream. Changes must reviewed before merging into this branch by a Fleetie, similar to merging into |
There was a problem hiding this comment.
| easily synced with upstream. Changes must reviewed before merging into this branch by a Fleetie, similar to merging into | |
| easily synced with upstream. Changes must be reviewed before merging into this branch by a Fleetie, similar to merging into |
| the upstream package directly via normal dependency management mechanisms (e.g. go dep or npm). Once this change has | ||
| been merged to `main` in the monorepo, the fork should be archived, with the repo description updated to indicate that | ||
| the upstream repo should be used instead. | ||
|
|
There was a problem hiding this comment.
Propose adding something like this:
| ### General Note | |
| Fleet historically has not kept third-party dependencies consistently up to date. Introducing forks may further increase divergence risk from upstream, potentially leading to challenging merges and integration issues. To manage this, we should schedule regular (e.g., monthly or quarterly) reviews to proactively synchronize our forks with upstream changes. | |
| `main` in `fleet/fleetdm` (e.g. Go changes must be reviewed by a backend engineer). | ||
|
|
||
| Periodically, branches mirroring the upstream repo should be synced to that repo. If upstream changes make sense to | ||
| merge into the Fleet-specific branch, an issue (engineering-initiated or otherwise as appropriate) should be created |
There was a problem hiding this comment.
Feels a bit heavyweight to create a separate issue. We don't create an issue every time we want to update a normal dependency. I try to update dependencies when I'm touching them.
| * Inconsistent conventions on where inlined dependencies live | ||
| * Difficult to incorporate upstream improvements | ||
| * Difficult to quickly determine drift between our version of the dependency and upstream | ||
| * Upstream version history is not reflected in our version of the code | ||
| * Contributing upstream is hard enough that it's unlikely to happen |
There was a problem hiding this comment.
Many of these cons can be addressed by having a standard process for updating a vendored dependency, as demonstrated in these UPDATE_INSTRUCTIONS
However, such a process is not an industry standard.
There was a problem hiding this comment.
@getvictor the update instructions are brilliant but it's hard to compete with git pull
lukeheath
left a comment
lukeheath
left a comment
There was a problem hiding this comment.
Thanks for writing up this ADR! I appreciate the opportunity to document a conversation and decision that's come up a few times over the past few years.
We have a strong bias for a single repo at fleetdm/fleet because of the reasons listed in the "Why do we use one repo?" section of the handbook. Over time, the benefit of additional repos has outweighed the cost, which is how we ended up with the five we have now.
Unfortunately, because every fork has to be a standalone repository, we've determined in the past that the cost of adding many new repos is greater than the cost of manually patching from upstream when needed.
Everything is always a draft at Fleet, so this may change in the future if the cost of keeping them in the monorepo is greater than the cost of manually patching them, but I don't think we're at that point yet.
|
@lukeheath Of note, this decision will have a chilling effect on pulling in external dependencies at all, leading to wheel-reinventing dev work, on top of the downsides mentioned in the ADR. Given that our answer to this question may change at some point, It'll be useful to have a quantitative threshold on when to bring this discussion up again. Maybe next steps would be explicitly tracking "downstreaming" work performed on modified third-party dependencies, and bringing this back up when that work takes, for example, at least one engineer-week in a sprint? This won't count the chilling effect mentioned above, which means that this heuristic will understimate the amount of extra work we're doing due to this monorepo adherence, but given the preference for the status quo overshooting is preferable to bringing this up for discussion earlier, only to no-op again. |
Co-authored-by: Luke Heath <luke@fleetdm.com>
|
Whoops, closed the PR rather than modifying to rejected. Assuming the intent here is that we merge rejected ADRs as rejected for paper trail reasons? |
|
We've been closing rejected ADRs so far. However, I think we should document a rejected ADR since the decision not to do something is also a decision. Either:
OR
cc: @lukeheath |
for #30109 # Details This PR fixes an issue in our current SQL parsing library that was causing queries like this to be marked invalid: ``` SELECT * FROM table_name WHERE column_name LIKE '\_%' ESCAPE '\' ``` This is valid in SQLite because the `\` is not considered an escape character by default. From [the SQLite docs](https://www.sqlite.org/lang_expr.html) (see section 3 "Literal Values (Constants)"; emphasis mine): > A string constant is formed by enclosing the string in single quotes ('). A single quote within the string can be encoded by putting two single quotes in a row - as in Pascal. C-style escapes using the backslash character are not supported because they are not standard SQL. # Use of forked code Part of the fix for this was [submitted as a PR to the node-sql-parser library](taozhi8833998/node-sql-parser#2496) we now use, and merged. I then found that another fix was needed, which I submitted as [a separate PR](taozhi8833998/node-sql-parser#2512). As these fixes have yet to be made part of an official release of the library, I made a fork off of the release we were using (5.3.10) and bundled the necessary build artifacts with Fleet. We have an [ADR proposing the use of submodules for this purpose](#31079); I'm happy to implement that instead if we approve that, although for a front-end module with a build step it's a bit more complicated. Hopefully this code will be released in `node-sql-parser` soon and we can revert back to using the dependency. Here is the [full set of changes](taozhi8833998/node-sql-parser@master...sgress454:node-sql-parser:5.3.10-plus). # Checklist for submitter - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [X] Manual QA for all new/changed functionality
|
@iansltx Agreed, it will/does have a chilling effect in that it's more difficult for us to add and maintain dependencies. Good idea to track the work resulting from this decision. I've created a @getvictor Agreed, we should set the status to "Rejected" and merge it as a record for future conversations. Instead of listing the specific reason, let's try just referencing the PR comment discussion. That way, full context for the decision can be shared vs. a small summary. |
lukeheath
left a comment
lukeheath
left a comment
There was a problem hiding this comment.
Actually before we can merge we need to back out the why-this-way.md changes.
|
Cool, I'll back out WTW Monday. |
|
@lukeheath Actually, revert is done now. Thx! |
4 participants
No description provided.