[foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.
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.
Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.
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, 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.
Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.
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, 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
Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.
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 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, 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 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.
Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.
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: _..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 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 > 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, 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 http
Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.
Wow this is nice! I did not see this coming... LZ On Sun, May 7, 2017 at 3:58 PM, Shimon Shtein 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: > _..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 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 >> 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, 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. >>
Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.
+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 wrote: > Wow this is nice! I did not see this coming... > > LZ > > On Sun, May 7, 2017 at 3:58 PM, Shimon Shtein 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: > > _..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 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 > >> 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, 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) > >> >> > >> >