+1 for fixing the integrity during plugin uninstallation. Option "A" seems to be clean and most easy to maintain.
On Tue, May 9, 2017 at 4:21 PM, Lukas Zapletal <[email protected]> wrote: > Wow this is nice! I did not see this coming... > > LZ > > On Sun, May 7, 2017 at 3:58 PM, Shimon Shtein <[email protected]> wrote: > > > > 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 <[email protected]> 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 <[email protected]> > >> 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, <[email protected]> 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 [email protected]. > >> >> 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 [email protected]. > >> > 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/topic/foreman-dev/51oBYLZpw80/unsubscribe. > >> To unsubscribe from this group and all its topics, send an email to > >> [email protected]. > >> 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 [email protected]. > > For more options, visit https://groups.google.com/d/optout. > > > > -- > Later, > Lukas @lzap Zapletal > > -- > 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 [email protected]. > 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
