[lockfile] Fix built-in plugin issues #2189
Conversation
internal/devbox/packages.go
Outdated
| @@ -270,6 +270,16 @@ func (d *Devbox) ensureStateIsUpToDate(ctx context.Context, mode installMode) er | |||
| ux.Finfo(d.stderr, "Ensuring packages are installed.\n") | |||
| } | |||
|
|
|||
| if mode != ensure { | |||
| // Reload includes because added/removed packages might change plugins. Cases: | |||
| // * New package adds built in. We wanna make sure the plugin is in config. | |||
There was a problem hiding this comment.
... adds built-in plugin...
internal/devbox/packages.go
Outdated
| if err := d.PluginManager().UpdateLockfileVersion(pluginConfig); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I don't know if this is the best place to add this, because we'll be updating the lockfile before the packages are actually installed in the nix store. In the event of an installation error, then the lockfile would be (wrongfully) updated.
In ensureStateIsUpToDate, we:
- call
d.installPackages() - ...// other operations that may also error
- call
d.updateLockfile
Could or should we move this pluginManager.updateLockfileVersion to d.updateLockfile()?
There was a problem hiding this comment.
yeah that makes sense. I put it here because I wanted to keep the existing logic intact (i.e. d.PluginManager().CreateFilesForConfig was previously doing this).
I think putting at the end is good. Another question is if we should move CreateFilesForConfig to the end as well, but at least for this PR, I'm inclined to leave as is.
internal/devbox/packages.go
Outdated
| if mode != ensure { | ||
| // Reload includes because added/removed packages might change plugins. Cases: | ||
| // * New package adds built in. We wanna make sure the plugin is in config. | ||
| // * Remove built-in that installs multiple packages (e.g. nginx). We wanna clear them up so |
There was a problem hiding this comment.
.. Remove built-in plugin ...
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Summary
Addresses #2187 and other issues.
Fixes:
addand instead was getting added on nextensureState. Fixed this by ensuring plugin version added during add.How was it tested?