fix(dev): fix reload error when using templates #5329
Conversation
|
Note: I never did quite repro the error Eythor originally reported (the errors I got were different), but it looks like it stems from the same cause (the templates not existing on the Garden instance in question). |
thsig
force-pushed
the
dev-mode-templates-fix
branch
from
0022910 to
8a529fb
Compare
|
My first reaction would be to go for option 1 here unless there are use cases for 2 that I'm missing. Isn't that something that could live on the I'm wondering if we should still merge the hack though since this frequently crashes running Garden processes. |
thsig
force-pushed
the
dev-mode-templates-fix
branch
from
8a529fb to
2df761b
Compare
|
With the unit test to cover the behaviour, I think this is OK for now. I didn't feel that a larger surgery on the state management around the instance manager was worth the effort and potential breakages at this point, given the lack of testing around this subsystem in general (also, the config scanning piece may be the main piece of shared state we'll have to deal with in this context). So I hope this is a practical, simple solution to the problem. |
core/src/commands/base.ts
Outdated
| @@ -287,7 +287,6 @@ export abstract class Command< | |||
| return withSessionContext({ sessionId, parentSessionId }, () => | |||
| wrapActiveSpan(this.getFullName(), async () => { | |||
| const commandStartTime = new Date() | |||
| const server = this.server | |||
There was a problem hiding this comment.
We really need a lint rule for "no-unused".
| * key errors when adding the newly scanned ones (and those generated from newly scanned config templates). | ||
| */ | ||
| private clearConfigs() { | ||
| for (const kind of Object.getOwnPropertyNames(this.actionConfigs)) { |
There was a problem hiding this comment.
Pardon me ignorance but why use getOwnPropertyNames instead of keys?
| * Essentially a vanilla object analog of `map.clear()` for ES5 Maps. | ||
| */ | ||
| export function clearObject<T extends object>(obj: T) { | ||
| for (const key of Object.getOwnPropertyNames(obj)) { |
thsig
force-pushed
the
dev-mode-templates-fix
branch
2 times, most recently
from
2b67422 to
4c7e2b9
Compare
|
Approving but we need to resolve conflicts. |
|
Bumping this one @thsig :) Could you look into resolving the conflicts so we can get this merged? |
|
Ah yes, will do! |
thsig
force-pushed
the
dev-mode-templates-fix
branch
4 times, most recently
from
d4f43e6 to
6f4b9b6
Compare
Before this fix, changing a Garden config during a `dev` command session and running two subsequent commands would result in the first command succeeding but the second failing with an error like: "Build foo references template <some-template> which cannot be found. Vailable templates: <None>" This was because the config templates scanned by the first subcommand after the config was changed weren't propagated to the parent Garden instance, which was then cloned and passed to the second command (which then had action configs, but no templates). This was fixed by making the relevant instance variables on the Garden class readonly (since reassignments in a child instance prevent newly scanned configs from propagating to the parent via the shared data structure) and emptying the maps before rescanning instead. Also added a unit test case that checks this whole flow.
The call to `this.clearConfig()` in `Garden#scanAndAddConfigs` had started breaking certain test cases that manually set action configs before calling `garden.getConfigGraph` (which would scan and add configs, thus discarding the manually set configs). While it's not great that we're adding more tweaks to the `TestGarden` class, we're almost never interested in explicitly testing the post-edit config reloading (outside of tests that test interactive / dev command funcftionality).
thsig
force-pushed
the
dev-mode-templates-fix
branch
from
6f4b9b6 to
cbb7acf
Compare
What this PR does / why we need it:
Before this fix, changing a Garden config during a
devcommand session and running two subsequent commands would result in the first command succeeding but the second failing with an error like:"Build foo references template which cannot be found. Vailable templates: "
This was because the config templates scanned by the first subcommand after the config was changed weren't propagated to the parent Garden instance, which was then cloned and passed to the second command (which then had action configs, but no templates).
This was fixed by making the relevant instance variables on the Garden class readonly (since reassignments in a child instance prevent newly scanned configs from propagating to the parent via the shared data structure) and emptying the maps before rescanning instead.
Also added a unit test case that checks this whole flow.
Which issue(s) this PR fixes:
Fixes #5290
Special notes for your reviewer:
This fix is rather hacky. We really shouldn't be relying on shared references to data structures in the instance variables of parent & child Garden instances. This happens to mostly work, but could easily lead to hard-to-diagnose problems in the future.
I thought I'd open the PR to start a discussion on how to proceed properly here.
Would it be better if we stored the action configs and config templates in an instance of a singleton class, to make sure there's ever only one version of the truth (the most up-to-date)?
Or do we explicitly want different child Garden instances to have their own copy of the configs & templates?
I think the choice boils down to either of these two:
ConfigRegistrythat's shared by all Garden instances. This would be populated/updated whenever configs are scanned, and there would always be exactly one instance of it in existence at any given time.The former sounds like a better approach to me, but I'm all ears.
CC @eysi09 @edvald