OK, it wasn't exactly my intention, but I see the discussion moves towards general plugin install/uninstall procedure.
I have discovered a nice feature that enables us to isolate plugin migrations, and run them on per-plugin manner: Apparently we can run: rake db:migrate SCOPE=my_plugin rake db:migrate SCOPE=my_plugin VARSION=0 # to migrate down And it will run only migrations that are tagged for that scope. One can achieve that by generating proper migration names: <timestamp>_<migration_name>.<scope/plugin_name>.rb I have added a PR that generates migrations for plugins with the proper naming: https://github.com/theforeman/foreman/pull/4509. This should be the base for complete removal. Then, if we add a down-only migration that removes STI's - it should be enough to remove all DB traces for a plugin. On Wed, May 3, 2017 at 2:11 PM, Lukas Zapletal <l...@redhat.com> wrote: > I agree with Tomas, it's more cleaner to remove all the data right > away. Therefore I suggest that we check for these kind of objects > during initialization and if such an class is (not) found, then we can > throw an error like "Run foreman-rake plugin:clean" to delete orhpaned > records. > > I am for (A) - remove all the data. > > LZ > > On Wed, May 3, 2017 at 11:16 AM, Tomas Strachota <tstra...@redhat.com> > 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. > > > > -- > Later, > Lukas @lzap Zapletal > > -- > You received this message because you are subscribed to a topic in the > Google Groups "foreman-dev" group. > To unsubscribe from this topic, visit https://groups.google.com/d/to > pic/foreman-dev/51oBYLZpw80/unsubscribe. > To unsubscribe from this group and all its topics, 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.
