-
Notifications
You must be signed in to change notification settings - Fork 3
implement backends interface and nats afero.FS and nats watcher. #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR implements a backends interface that allows different afero.FS backends and custom watchers for hot reloads, along with a NATS-based implementation. It also adds the ability to start xtemplate without templates loaded initially, enabling templates to be added after startup.
- Introduces a new backends interface with filesystem and NATS Object Store implementations
- Adds support for starting servers without initial templates (graceful degradation)
- Updates the provider initialization system to support backend creation
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| config.go | Adds Backend field and duplicates CrossOrigin field |
| backends/ | New backend interface and implementations for filesystem and NATS Object Store |
| server.go | Updates server creation to handle missing templates gracefully and backend management |
| instance.go | Refactors provider initialization order and backend setup |
| app/app.go | Updates watcher setup to use backend-provided watchers |
| test/ | Adds NATS test application and configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
uh oh, i rebased on top of your most recent few commits just before submitting - it seems like there's some compiler errors. I'll push a new one soon |
c75f816 to
e52e355
Compare
|
Ok, i resolved a few merge conflicts, as well as added
|
|
looks like the check failed because it cant log into docker hub |
|
Thanks for opening this, Nick! I think this could be an interesting capability to have in xtemplate. Scanning through the diffs it looks pretty good so far. To share my first live reaction, I'm a bit hesitant about adjusting the order of operations: initializing the providers first, which can modify the backend state, how that affects reloads etc. I don't see any obvious problems yet, but this stuff is tricky so want to consider the implications. Don't worry about CI, getting CI to work with contributor PRs requires reps to iron out the kinks, I'll look into it. I will pull your branch to review more carefully and test locally this week anyway. |
|
Yeah, it's all tricky for sure. I really did my best to not make any changes at all to xtemplate, and in fact there initially weren't any meaningful changes. But as I tested things I kept finding edge cases and thats where things like the re-ordering, adding pointer to nats config, adding config param to dot providers etc came in. I also initially had a WithBackend method to add a backend, but then opted to bake it into WithNats given that it's somewhat inseparable from the nats provider, given that they'd want/need to use the same nats server. But it just occurred to me that we probably would want to add back the WithBackend for use with other potential backends (s3, git etc) that perhaps don't rely on a provider. I'll push a commit to add it back today. It is entirely possible that there's other, better ways to do it all, but I think I'm tapped out on insights (and energy). You know the codebase and vision far better than I do, so if you can think of a better way to do anything, I'd be happy to make changes. It's not urgent that this be merged - I'm going to focus on some other stuff for a little while. So, get to it whenever you have time! |
3052953 to
e97ced8
Compare
…plementations Add cue hurl tests for watcher and reload. Allow starting xtemplate without any templates loaded.
e97ced8 to
636d8b6
Compare
|
Nice, CI passes now. 🎉 I took the liberty to pull out some pieces of this PR into the
Some other changes: fixed CI for external contributors, and adjusted recent commit messages on the next branch and on this PR. As for what remains:
|
|
ah, glad youre receptive to mise! It's incredible. I originally used the go backend, which just seems to run I might as well also mention jujutsu vcs and its jjui TUI - complete "gamechangers" for doing version control. On that note, I hope it wasn't too much effort for you to extract the linting fixes - I could have done it fairly easily if you had asked for it, as jj has an interactive split mode that makes it dead-simple to extract specific lines and sections of code that have changed. I'm very much an evangelist for jj, jjui and mise, spreading the Good Word wherever I can. for the remaining tasks, I'm completely open to any refactoring that you want to do - I was trying to be as non-invasive as I could, but if you're willing to change xtemplate in various ways, im sure that there's better, simpler, cleaner ways to implement all of this. I'm happy to receive suggestions, but its probably best if you do it as you understand the codebase, vision, what you want to maintain etc... I'd like to think that I've at least done the heavy lifting of implementing the NATS afero.FS and watcher, and showing a working PoC of how to use them with interfaces.
Anyway, glad you're mostly receptive to all of this. I look forward to seeing what it becomes! Let me know if I can help with anything. |
|
I'm pretty familiar with git so it wasn't too much trouble to shuffle some hunks around (fine I admit AI helped a bit). Everyone raves about jj and I'm sure it's great, but I didn't dedicate enough time to become proficient and screwed up my branches with it. Trying it again is on the list.
Yes that's the idea.
xtemplate loads all files into a single instance of It may not look like it, but template definitions are code. Imagine taking a ruby on rails app and checking out a random subset of ruby files from one commit and the rest of the files from the next commit and running it. There is no telling what the result could be, everything could be mostly fine or there could be a site breaking bug or a giant security vulnerability, even if each commit in isolation works perfectly. I'm doing this full-reload approach because I do not want to play with this kind of issue. Honestly even the current debounce-based reload is risky. There might be a way to cache intermediate template parse results, so the Do you see how being loosey-goosey with template loading could be a huge problem?
One could say I've already drunk the coolaid, since nats is embedded and configurable in all xtemplate deployments today. You can either let xtemplate create your nats client and optionally server (per instance because that's the configuration/execution state boundary used by xtemplate) OR you can provide an already-connected client in which case xtemplate will just use it. If you want a persistent server, instead of pushing creation and management of the server down into the xtemplate module where there's some impedance mismatch with how xtemplate manages instance state, persistent things should be created by the caller (i.e. your own
Yes CUE cmd is tricky to write, honestly I'm impressed that you got it working! CUE is already very parallel since everything runs in parallel by default unless you explicitly sequence with |
|
re: full reload. Fair enough! Thanks for the context. It seems sensible as-is. yes, i agree that a persistent nats server - embedded or otherwise - would make more sense in /app or /cmd. I actually first implemented it like that and passed it through, but then I found dot_nats_config and figured I'd try to integrate it there. In fact, it seems to me that my first cut with everything was actually the cleanest - i only started getting invasive when I started discovering other mechanisms in xtemplate and trying to integrate with them. I dare not try to dig it out of old commits though - things got messy, despite jj. If you can think of a way to extract the embedded server from the config to the app/cmd, but in a way that is still easily configurable/importable, that would be great! Ah ok, I didnt realize (or perhaps remember) that Cue is parallel. Perfect! Mise can just install tools then. Let me know how i can help with anything! |
|
Interesting to hear about the history of this patch. Yes, the hardest part of developing xtemplate has been deciding exactly where to cut in features, it has a bunch of layers that are different from typical apps so it's often unclear where features should live until I play with a few different approaches.
🫡 I will try, wish me luck! We discussed how you'd like xtemplate to enable your desired architecture in #63, but we didn't really get into the how and why, and to be honest I'm still a bit puzzled why you want this feature. Maybe it would be helpful if you expanded a bit on how you hope to use the nats object store backend: What does the site do? Who are the users? Where to do the generated templates come from? Why do they change so often? What do they contain? To be clear, I'm not trying to qualify the feature, I'm just curious. |
|
I'm not at all trying to be evasive, but its hard to describe what exactly I'm trying to do... I won't really be exposing any of this to end users (though, I strongly suspect that @joeblew999 will want to do something like that - we're friends so I'm familiar with what he's up to). But, in essence, I just want to be able to somewhat dynamically generate templates from a UI and/or other programattic mechanisms, as part of a fairly dynamic, reactive web application - they are unlikely to ever be (or at least not primarily) actual files in the filesystem. My constraints/needs are that I want it to be
Is this at all helpful? It seems to me that its just a good idea in general, regardless of the weird things im doing in my app. |
|
I’m building this thing to keep all files in an S3 and for each serve to pull them to local disk only when they are needed . so local fs is really a cache . Using S3 ( I use R2 ) is important for me so that servers stay stateless . it’s in my repo but not finished. it’s generic so could be useful with xtemplate. if anything changes S3 , A simple HTTP hooks fired off R2 so that all nodes know too and will update local cs he is needed. If you mutate local file , it will make sure S3 ( R2) updates . So xtemplate is working off Local FS, but files are really on S3 |
Closes #63
This PR adds a backends interface that allows for using different afero.FS backends along with providing a custom Watcher to allow for hot reloads. It also implements a NATS-based afero.FS and Watcher.
Made changes to allow starting xtemplate without any templates loaded, so that templates can be added after startup.
It comes with an "app-nats" test app that is run by
cue cmd build_test_nats, which populates the nats object store with templates after it has already initialized, triggering a hot reload. Then it runs the hurl test suite against it. The other tests (or at leastbuild_test_cli) might want to be modified to test the watcher and reload as well. I assume you could polish off the CUE stuff - I dont have a clue about how it truly works.Also fixed a bunch of small lint errors (sorry, i should have done that in a separate commit at the very least, if not PR...)
It is important to note that I built this on top of your WIP
nextbranch, and am merging it there. If you want to wait til you merge that to main, that is fine.I also added a
mise.tomlfile, which allows for easily installing all of the required tooling (caddy, xcaddy, nats, etc..). Mise is absolutely beautiful, and the future of such tooling as far as I'm concerned.If there's any changes that you dont understand or have suggestions for changes, please let me know! I could retroactively create various commits, or even split to different PRs if that would help you to review this.