Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.

[Lukas Zapletal]

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 

Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.

[Shimon Shtein]

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 

Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.

[Lukas Zapletal]

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.

[Marek Hulán]

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 

Re: [foreman-dev] [RFC] Proposal: make foreman STI to work even when an inherited class is not found in Ruby.

[Tomas Strachota]

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.