Thanks for bringing this up. I think the right approach is to have plugin uninstallation or rather clean up rake tasks. The clean up should IMHO delete all data that would case problems after code removal. It shouldn't try to revert the schema changes, but destroy all data that can cause problems.
This was added recently to foreman_templates [1] and it would be great if other plugins would follow the same pattern. I think core could provide a helper for common clean tasks, such as removing custom settings, but only plugin author knows what needs to be cleaned so custom task will always be required. The complete plugin uninstallation means running clean up task and then removing the plugin package or bundler dependency. If we inform user that clean up task will destroy data and they should do a backup I think we can avoid all hacks to make STI work even without class definitions. From my experience this only leads to hard to debug issues. So looking at options Tomas listed, I'd prefer a). In terms of where they should live, I'd prefer plugin repos since only maintainers of plugin knows what needs to be cleared. Core could provide common helpers that plugin rake tasks could use. We'll see after we have it for few plugins what would be a good candidate. Foreman maintain could have checks and options for uninstalling the plugins using these rake tasks. I'd like to avoid introducing this in the installer, puppet does not seems to be the tool that is good fit for uninstallation and adding it via hooks sounds like hack. [1] https://github.com/theforeman/foreman_templates/pull/44 -- Marek On středa 3. května 2017 11:16:54 CEST Tomas Strachota wrote: > This issue is tightly coupled with plugin uninstallation and I think > we should solve the two problems together. At the moment there's no > standard plugin uninstallation procedure that would take care of > cleaning up the system. This is something each plugin should provide. > One thing (imho the less important in this context) is where we put > such code. Should it be a rake task, part of installer, part of > maintanance script, somewhere else? > > What I see as more important is to decide what data will the > uninstallation process remove (keep all vs. keep only settings vs. > wipe all) and how we want the plugin to behave if it's installed > again. This will affect how we approach missing STI classes. > > I can see 3 options: > > a) Remove all data > Plugin would remove all tables and records it created. In such case we > probably don't need to change the STI behavior. Plugin uninstallation > should take care of removing the data correctly. If it fails it's fine > to throw exception to indicate the system integrity is broken. This > approach is imho safer for us as developers and requires less > defensive coding. > > b) Keep all the data > In this case the original data should be present when the plugin is > installed again. STI records without classes should be completely > hidden in the default scope. If all records are listed we should > return either instances of base class or some special class for the > missing types. I'm afraid this approach is quite fragile and can lead > to many surprises when a plugin is uninstalled, the foreman lives > without it for some time and then you install it again. I'm also not > sure how common is such usecase. > > c) Combination of a) and b) > We can keep data where it's safe (like settings) and delete the rest. > > I'm in favor of a) or c) > > T. > > On Sun, Apr 30, 2017 at 10:05 AM, <ssht...@redhat.com> wrote: > > Hello, > > > > lately I was switching plugins on and off, and I stumbled upon an annoying > > issue: Many plugins that add some STI classes (for example Katello that > > adds settings, or Discovery that adds DiscoveredHost). > > A problem starts when such a plugin is removed from the system, since > > default STI implementation will throw an error if it can't load a class > > that corresponds to the :type column in the DB. > > > > I propose a custom handling for such cases, since they are not exactly > > exceptions in our system design. > > I was thinking about one of the following options to handle this case, and > > would like to see a voting which one you prefer. Of course other options > > can be proposed too. > > > > 1. Return a base class for such records (Maybe with an error already set) > > 2. Return nil/some kind of special AR object for such records (will > > require > > defensive coding while handling heterogeneous lists) > > 3. Filter those records out with default scope (will require more > > declaration in plugins, or more DB queries on system startup) > > > > Any thoughts in this area will be much appreciated, > > > > Shim > > > > -- > > You received this message because you are subscribed to the Google Groups > > "foreman-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to foreman-dev+unsubscr...@googlegroups.com. > > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "foreman-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
