update bundles in place when in dev mode #1006
Conversation
mpeck
force-pushed
the
peck/update-config
branch
2 times, most recently
from
bb5db67 to
5469e69
Compare
lib/cog/repository/bundles.ex
Outdated
| |> Repo.preload(:templates) | ||
|
|
||
| Enum.each(new_version.commands, &Repo.delete!/1) | ||
| Enum.each(new_version.templates, &Repo.delete!/1) |
There was a problem hiding this comment.
Why are you deleting the commands and templates of the thing you just installed?
There was a problem hiding this comment.
The old commands and templates are removed here and they are reinstalled later in the transaction. We could go through and update all of the associations instead of just reinstalling, but that is a lot more logic and seemed more likely to introduce bugs. With this method bundle installation continues just as it did normally with the exception that we reference the old bundle version instead of a newly created one.
There was a problem hiding this comment.
That's not clear from this... you're calling delete on the commands and templates from new_version.
There was a problem hiding this comment.
Actually, the functionality should be more like "delete the existing bundle at version X, and then install this new "version" of version X". We already handle the associations properly that way.
However, for it to be truly transparent, you're also going to have to deal with enabled / disabled status.
There was a problem hiding this comment.
Hrm, that would be cleaner. I did it like this to preserve the id of the bundle version. That way we wouldn't have to go through and clean up references to it. But as I'm looking again you're right. The only one to worry about is the enabled version. Everything else references the bundle id.
Let me try that and see how if everything works like I'm thinking.
|
|
||
| Application.put_env(:cog, :enforce_bundle_version, true) | ||
| end | ||
|
|
There was a problem hiding this comment.
I'm concerned that there are a lot of edge cases in overall behavior that we're not addressing with this test. I'm thinking in particular of the rules and permissions associated with the old and new bundles.
There was a problem hiding this comment.
Yeah, I agree. I just did a quick smoke test to validate it was working. If you think my general approach is reasonable, I'll add some more tests to make sure everything is working as expected.
|
Hmmm. I envisioned this feature working per-bundle not a global switch for the Cog install. Something like:
|
|
@kevsmith I can do that. In that case would it be a feature that's always available, or should we require a setting in Cog to enable it? |
|
Always available. Think of it like |
mpeck
force-pushed
the
peck/update-config
branch
2 times, most recently
from
dd79417 to
9fb59c5
Compare
|
@kevsmith @christophermaier how does this look? and as a sister PR, cogctl support operable/cogctl#93 |
mpeck
force-pushed
the
peck/update-config
branch
from
9fb59c5 to
c230729
Compare
|
Looks good. Squash the commits down and |
mpeck
force-pushed
the
peck/update-config
branch
from
c230729 to
152c270
Compare
|
@kevsmith |
|
@mpeck Disregard. I missed the enclosing transaction on my first read thru. |
|
One other question -- what does the error look like if a forced install fails? |
|
@kevsmith The installation path is basically identical as normal. The only difference being that we delete the old version before installing the new one. I tried to keep everything as similar as I could. So installation errors are the same regardless. |
|
Ok. The reason I ask is because we had issues in the past with errors inside a function running in a transaction (I think in rules ingestion) generating pretty cryptic error messages. I agree w/ @christophermaier |
Was having a hard time explaining what I was thinking so I thought I would just do it. This adds an extra config that causes bundle installation to overwrite the existing version if it already exists.
resolves #969