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.
Return a base class for such records (Maybe with an error already set)
Return nil/some kind of special AR object for such records (will require
defensive coding while handling heterogeneous lists)
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,
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+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
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.
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.
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.
Return a base class for such records (Maybe with an error already set)
Return nil/some kind of special AR object for such records (will
require
defensive coding while handling heterogeneous lists)
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,
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+unsubscribe@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+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
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 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 tstracho@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.
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.
Return a base class for such records (Maybe with an error already
set)
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
···
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+unsubscribe@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+unsubscribe@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/topic/foreman-dev/51oBYLZpw80/unsubscribe.
>> To unsubscribe from this group and all its topics, send an email to
>> foreman-dev+unsubscribe@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+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
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:
_<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 lzap@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 tstracho@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.
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.
Return a base class for such records (Maybe with an error already
set)
Return nil/some kind of special AR object for such records (will
require
defensive coding while handling heterogeneous lists)
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
