-
Notifications
You must be signed in to change notification settings - Fork 82
Conversation
josephjclark
commented
May 7, 2026
Hi @jeremi ! Thanks for raising this. Before we ask anyone to take a closer look I have big question:
Lets OPENFN_ADAPTORS_REPO accept a colon-separated list of paths so a private adaptor monorepo can be loaded alongside the canonical OpenFn adaptors monorepo without forking.
What is the use-case for this?
I understand the need to develop an adaptor from a fork in the repo. But if you've forked the repo, why would you need the OG adaptors repo and your fork? Surely the fork has has everything that adaptors/main has?
Maybe if you can better explain the requirement we'll be better able to accommodate the request :)
Hi Joseph, the use case isn't forking OpenFn/adaptors. It's running a separate adaptors monorepo we manage alongside the canonical one, without touching either. Public or private doesn't matter; the point is it's ours to manage.
With colon-separated OPENFN_ADAPTORS_REPO the canonical repo stays pristine, and our adaptors live in their own repo with their own release cadence. Same composition pattern as PATH. First-wins also lets us shadow an upstream adaptor for a quick experiment or local fix without committing to a fork.
And if an adaptor matures into something the wider community could use, it's a clean handoff to the canonical repo since the package layout already matches.
taylordowns2000
commented
May 8, 2026
hey @jeremi , i think i get it. in essence this would allow you to more easily host your own set of adaptors alongside the openfn adaptors, and use both at the same time without worrying about constantly rebasing your fork (you wouldn't need a fork at all) on the official base?
i like this, as we could envision some national government system specific adaptors living in a totally separate (private, never published) repository owned and maintained by ministry X, Y, or Z.
one thing (that needn't necessarily be addressed in this PR, but definitely in later PRs) would be making it possible to use a local repo and the standard npmjs path. the most common use case would be: "i'd like to boot up lightning and use all the regular adaptors via the regular npmjs delivery mechanism and also (rather than instead of) use these 5 private adaptors i've built myself and have no intention of adding to the official monorepo."
☝️ that feels like a really nice, simple use case. is this where this is going, in your mind?
jeremi
commented
May 8, 2026
Yes, exactly. And for me it was more an issue as I am developing connector, and I remember having the same issue when we did the first version of the openspp adapter. This will make it easier.
Also that would allow you to not get all the adaptors in your repo and be responsible for their maintenance. Some could be core, some could be third party like in a lot of software.
josephjclark
commented
May 8, 2026
Ok - I'll take a closer look and get and get these two PRs reviewed as soon as I can. Are you in any hurry @jeremi ?
jeremi
commented
May 8, 2026
No, no hurry, thanks.
@josephjclark
josephjclark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've assigned @midigofrank for a review of the elixir code, but as mentioned in kit I think the paths need to comma-seperated, not colon seperated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docs are in the wrong place. Better to have them in runninglocal.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many docs here - I would remove all this. RUNNINGLOCAL.md explains the variables well enough, no need to bloat this file
jeremi
commented
May 26, 2026
@josephjclark Feel free to adjust as you think is better.
jeremi
commented
May 26, 2026
Pushed c136fce:
OPENFN_ADAPTORS_REPOnow splits on,instead of:, matching the ws-worker side (feat(ws-worker): support multi-root @local adaptor resolution kit#1397 ) and avoiding the Windows drive-letter collision. Single-path callers are unchanged.- Ran
mix formatto clear the CI lint failure (adaptor_registry.ex, the two test files it called out). - Updated
.env.exampleandRUNNINGLOCAL.mdto the comma form, and tightened the docs note about missing repos — they're now skipped with a warning rather than fatal, peradaptors_in_repo/1.
Ready for another look.
josephjclark
commented
May 27, 2026
Oh great @jeremi, thanks! I'll try and take a look at this today
@josephjclark
josephjclark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am generally happy with the PR but we cannot merge until:
a) the worker has been updated and released in kit
b) We work out what to do about an upcoming conflict in the registry.
Subject to speaking to @stuartc I may merge this into our repo on a branch so that we have control of it and can work out the best route to production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is super unfortunate timing - @stuartc is just at the tail end of re-writing this code.
@stuartc I hate to make your project more complicated, but what do you make of this change?
We can merge this to main and have you rebase to unblock Jeremi; or we can have you port this solution int your branch (which blocks Jeremi and increases the risk of this work)
Tricky one. We should talk about this tomorrow!
@stuartc
stuartc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jeremi, solid work.
My only comments here are around the:
- shadow warning, it's hard to understand on the warning, like:
2 adaptor(s) shadowed ...: @openfn/language-http, @openfn/language-http. On that and as an aside, we can collapse the reduce into something like this:
defp dedupe_first_wins(adaptors) do
kept = Enum.uniq_by(adaptors, & &1.name)
shadowed = adaptors -- kept
log_shadowed(kept, shadowed)
kept
end
defp log_shadowed(_kept, []), do: :ok
defp log_shadowed(_kept, []), do: :ok
defp log_shadowed(kept, shadowed) do
winner = Map.new(kept, &{&1.name, &1.repo})
grouped = Enum.group_by(shadowed, & &1.name, & &1.repo)
details =
Enum.map_join(grouped, "; ", fn {name, losers} ->
"#{name} (using #{winner[name]}, shadowed #{Enum.join(losers, ", ")})"
end)
Logger.warning(
"AdaptorRegistry: #{map_size(grouped)} package(s) shadowed: #{details}"
)
end
- I think a warning that a repo path (now that the ! was dropped from
File.ls!when the directory doesn't exist you silently get nothing in the dropdown, I think a warning that either the directory doesn't exist or that no adaptors were found there; the latter probably being easier since the path that is actually pointed at is different to the env.
josephjclark
commented
Jun 2, 2026
I've tested this against the kit branch and it works great. The worker will be released shortly.
While testing this I noticed that the updated credentials UI is basically totally broken in local mode. Unrelated to this Pr, but we should fix it #4822
OPENFN_ADAPTORS_REPO now accepts a colon-separated list of paths so that a private adaptor repo can be loaded alongside the canonical OpenFn adaptors monorepo. Order is precedence: when two repos ship a package with the same dirname, the entry from the earlier path wins and a single summary warning is logged for the shadowed entries. The bootstrap layer normalises the env var into the `local_adaptors_repos` list config key, which is the sole input the registry consumes. Single-path values still work; they just become a one-element list. A missing or unreadable repo path is logged as an error and skipped, so a typo in one entry does not take down the others. The bundled ws-worker still expects a single path for `@local` adaptor execution, so colon-separated values only widen the registry view (picker UI, metadata). RUNNINGLOCAL.md and .env.example document the limitation.
Colon collides with Windows drive letters (c:/repo); comma matches the ws-worker parser so the registry view and @Local resolution agree. Single-path callers are unchanged. Also applies mix format to satisfy CI and tightens the docs note about missing repos (they are skipped, not fatal).
c136fce to
b903ab1
Compare
Uh oh!
There was an error while loading. Please reload this page.
Summary
Lets
OPENFN_ADAPTORS_REPOaccept a colon-separated list of paths so a private adaptor monorepo can be loaded alongside the canonical OpenFn adaptors monorepo without forking. The registry merges all roots, with first-wins semantics on dirname collisions.Companion ws-worker PR
This PR is the registry half. The matching executor-side patch is OpenFn/kit#1397, which teaches
@openfn/ws-workerto walk the same colon list when resolving@localadaptors. The two PRs ship together so multi-root@localworks end-to-end.Note: the in-tree docs (
.env.exampleandRUNNINGLOCAL.md) and the commit message still describe the "registry-only, ws-worker single-path" state from before kit#1397 existed. Happy to amend in this PR or as a follow-up once kit#1397 lands and a new ws-worker version is published, whichever you prefer.End-to-end verification
Verified live in a Lightning dev container with both patches applied (
OPENFN_ADAPTORS_REPO=/private-adaptors:/openfn-adaptors):[info] Starting AdaptorRegistryfollowed by[warning] AdaptorRegistry: 1 adaptor(s) shadowed by earlier local-adaptors repo entries: @openfn/language-publicschema(the canonical entry was shadowed by the private root, as expected).@openfn/language-publicschema@localwas resolved by the patched ws-worker to/private-adaptors/packages/publicschema/dist/index.js(the first root in the colon list), with the runtime loggingResolved adaptor @openfn/language-publicschema to version 0.0.1.Open question for maintainers
The patch overloads
OPENFN_ADAPTORS_REPOwith colon-separated semantics, which is the path of least surprise for shell users (mirrorsPATH) but does change what the env var can mean. Happy to rename to a new variable (e.g.OPENFN_ADAPTORS_REPOS) if you'd prefer to keep the singular semantics on the existing one. Internally the bootstrap already normalises into a singlelocal_adaptors_repos(plural) config key, so the rename is purely an env-var concern.Test plan
mix test test/lightning/adaptor_registry_test.exs— 11 tests, all greenmix test test/lightning/config/bootstrap_test.exs— 50 tests, only the pre-existing kafka-misconfig flake (unrelated baseline failure onmain)mix test test/lightning_web/channels/run_with_options_test.exs— 4 tests, all greenmix format --check-formattedclean for the touched codemix credo --strict --allclean for the touched codeNew tests added:
local_adaptors_enabled?/0plural-key behaviour (true with non-empty list, false with empty list, false when key absent)OPENFN_ADAPTORS_REPO(single-path, multi-path, whitespace/empty-segment trimming)