Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Martin Basti

On 13/03/15 11:55, Petr Spacek wrote:

On 13.3.2015 11:34, Jan Cholasta wrote:

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be
executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.




Ideally we want to fix all plugins to dry-run the upgrade not
just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.


I agree that this breaks the spirit of --test and think it
should be
fixed before committing.

Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We can
re-visit it
later on, open a ticket :-)


No. There is no rush for this, at least not for the promise of a
future
fix that will never come.

I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
 log message
else
 do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a
fix now
than a ticket that would never be done.

I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.

I like idea about mock_writes=True. However, I think we still need to
make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.

That is exactly why I mentioned LDAP transactions. There is no other
way how
to test complex plugins which actually read own writes (except mocking
the
whole LDAP interface somewhere).

While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test mid-term. I
currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
- search for all unfixed DNS zones
- fix them with ldap update
- was the search truncated, i.e.  are there more zones to update?
  if yes, go back to start

  - Make the plugins not call {add,update,delete}_entry themselves but rather
return the updates like they should. This is what the ticket
(https://fedorahosted.org/freeipa/ticket/3448) requests and what should be
done to make --test work for them.

How do you propose to handle iterative updates like the DNS upgrade mentioned
by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?

So instead of DNS commands logic, which can be used in plugin, we should 
reimplement the dnzone commands in upgrade plugin, to get modlist? And 
keep watching it and do same 

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Martin Kosek

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be executed
in --test mode.

This patch is a workaround, to ensure update with --test option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that this
requires 'proper' fix now. It was broken for *years* so this patch is a huge
improvement and IMHO should be commited in current form. We can re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting 
ldap.update_entry calls in upgrade plugins with


if options.test:
   log message
else
   do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py: 
ldap.update_entry(uid_uniqueness_plugin)



So from my POV, very quick fix. In that case, I would also prefer a fix now 
than a ticket that would never be done.


Martin

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Petr Spacek
On 13.3.2015 10:18, Martin Kosek wrote:
 On 03/12/2015 05:10 PM, Rob Crittenden wrote:
 Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be executed
 in --test mode.

 This patch is a workaround, to ensure update with --test option will not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.




 Ideally we want to fix all plugins to dry-run the upgrade not just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.


 I agree that this breaks the spirit of --test and think it should be
 fixed before committing.

 Considering how often is the option is used ... I do not think that this
 requires 'proper' fix now. It was broken for *years* so this patch is a huge
 improvement and IMHO should be commited in current form. We can re-visit it
 later on, open a ticket :-)


 No. There is no rush for this, at least not for the promise of a future
 fix that will never come.
 
 I checked the code and to me, the proper fix looks like instrumenting
 ldap.update_entry calls in upgrade plugins with
 
 if options.test:
log message
 else
do the update
 
 right? I see just couple places that would need to be updated:
 
 $ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
 ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(base_entry)
 ipaserver/install/plugins/update_managed_permissions.py: 
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
 ipaserver/install/plugins/update_referint.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_uniqueness.py:
 ldap.update_entry(uid_uniqueness_plugin)
 
 
 So from my POV, very quick fix. In that case, I would also prefer a fix now
 than a ticket that would never be done.

I really dislike this approach because I consider it flawed by design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update plugins and do
not add risk of forgetting about one of them when adding/changing plugin code.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Alexander Bokovoy

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be executed
in --test mode.

This patch is a workaround, to ensure update with --test option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that this
requires 'proper' fix now. It was broken for *years* so this patch is a huge
improvement and IMHO should be commited in current form. We can re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
   log message
else
   do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a fix now
than a ticket that would never be done.


I really dislike this approach because I consider it flawed by design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update plugins and do
not add risk of forgetting about one of them when adding/changing plugin code.

I like idea about mock_writes=True. However, I think we still need to make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.

--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Petr Spacek
On 13.3.2015 10:42, Alexander Bokovoy wrote:
 On Fri, 13 Mar 2015, Petr Spacek wrote:
 On 13.3.2015 10:18, Martin Kosek wrote:
 On 03/12/2015 05:10 PM, Rob Crittenden wrote:
 Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be executed
 in --test mode.

 This patch is a workaround, to ensure update with --test option will 
 not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.




 Ideally we want to fix all plugins to dry-run the upgrade not just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.


 I agree that this breaks the spirit of --test and think it should be
 fixed before committing.

 Considering how often is the option is used ... I do not think that this
 requires 'proper' fix now. It was broken for *years* so this patch is a 
 huge
 improvement and IMHO should be commited in current form. We can re-visit 
 it
 later on, open a ticket :-)


 No. There is no rush for this, at least not for the promise of a future
 fix that will never come.

 I checked the code and to me, the proper fix looks like instrumenting
 ldap.update_entry calls in upgrade plugins with

 if options.test:
log message
 else
do the update

 right? I see just couple places that would need to be updated:

 $ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
 ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(base_entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_pacs.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_referint.py:   
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_uniqueness.py:
 ldap.update_entry(uid_uniqueness_plugin)


 So from my POV, very quick fix. In that case, I would also prefer a fix now
 than a ticket that would never be done.

 I really dislike this approach because I consider it flawed by design. Plugin
 author has to think about it all the time and do not forget to add if
 otherwise ... too bad.

 I can see two 'safer' ways to do that:
 - LDAP transactions :-)
 - 'mock_writes=True' option in LDAP backend which would print modlists 
 instead
 of applying them (and return success to the caller).

 Both cases eliminate the need to scatter 'ifs' all over update plugins and do
 not add risk of forgetting about one of them when adding/changing plugin 
 code.
 I like idea about mock_writes=True. However, I think we still need to make
 sure plugin writers rely on options.test value to see that we aren't
 going to write the data. The reason for it is that we might get into
 configurations where plugins would be doing updates based on earlier
 performed tasks.  If task configuration is not going to be written, its
 status will never be correct and plugin would get an error.

That is exactly why I mentioned LDAP transactions. There is no other way how
to test complex plugins which actually read own writes (except mocking the
whole LDAP interface somewhere).

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Martin Kosek

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be executed
in --test mode.

This patch is a workaround, to ensure update with --test option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that this
requires 'proper' fix now. It was broken for *years* so this patch is a huge
improvement and IMHO should be commited in current form. We can re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
log message
else
do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a fix now
than a ticket that would never be done.


I really dislike this approach because I consider it flawed by design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update plugins and do
not add risk of forgetting about one of them when adding/changing plugin code.

I like idea about mock_writes=True. However, I think we still need to make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.


That is exactly why I mentioned LDAP transactions. There is no other way how
to test complex plugins which actually read own writes (except mocking the
whole LDAP interface somewhere).


While this may be a good idea long term, I do not think any of us is 
considering implementing the LDAP transaction support within work on this 
refactoring.


So in this thread, let us focus on how to fix options.test mid-term. I 
currently see 2 proposed ways:

- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it. Although 
thinking of this approach, I think it may make some plugins like DNS loop 
forever. IIRC, at least DNS upgrade plugin have loop

  - search for all unfixed DNS zones
  - fix them with ldap update
  - was the search truncated, i.e.  are there more zones to update?
if yes, go back to start

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Petr Spacek
On 13.3.2015 11:34, Jan Cholasta wrote:
 Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):
 On 03/13/2015 11:00 AM, Petr Spacek wrote:
 On 13.3.2015 10:42, Alexander Bokovoy wrote:
 On Fri, 13 Mar 2015, Petr Spacek wrote:
 On 13.3.2015 10:18, Martin Kosek wrote:
 On 03/12/2015 05:10 PM, Rob Crittenden wrote:
 Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be
 executed
 in --test mode.

 This patch is a workaround, to ensure update with --test
 option will not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.




 Ideally we want to fix all plugins to dry-run the upgrade not
 just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.


 I agree that this breaks the spirit of --test and think it
 should be
 fixed before committing.

 Considering how often is the option is used ... I do not think
 that this
 requires 'proper' fix now. It was broken for *years* so this
 patch is a huge
 improvement and IMHO should be commited in current form. We can
 re-visit it
 later on, open a ticket :-)


 No. There is no rush for this, at least not for the promise of a
 future
 fix that will never come.

 I checked the code and to me, the proper fix looks like instrumenting
 ldap.update_entry calls in upgrade plugins with

 if options.test:
 log message
 else
 do the update

 right? I see just couple places that would need to be updated:

 $ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
 ipaserver/install/plugins/dns.py:
 ldap.update_entry(container_entry)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(base_entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_pacs.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_referint.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_uniqueness.py:
 ldap.update_entry(uid_uniqueness_plugin)


 So from my POV, very quick fix. In that case, I would also prefer a
 fix now
 than a ticket that would never be done.

 I really dislike this approach because I consider it flawed by
 design. Plugin
 author has to think about it all the time and do not forget to add if
 otherwise ... too bad.

 I can see two 'safer' ways to do that:
 - LDAP transactions :-)
 - 'mock_writes=True' option in LDAP backend which would print
 modlists instead
 of applying them (and return success to the caller).

 Both cases eliminate the need to scatter 'ifs' all over update
 plugins and do
 not add risk of forgetting about one of them when adding/changing
 plugin code.
 I like idea about mock_writes=True. However, I think we still need to
 make
 sure plugin writers rely on options.test value to see that we aren't
 going to write the data. The reason for it is that we might get into
 configurations where plugins would be doing updates based on earlier
 performed tasks.  If task configuration is not going to be written, its
 status will never be correct and plugin would get an error.

 That is exactly why I mentioned LDAP transactions. There is no other
 way how
 to test complex plugins which actually read own writes (except mocking
 the
 whole LDAP interface somewhere).

 While this may be a good idea long term, I do not think any of us is
 considering implementing the LDAP transaction support within work on
 this refactoring.

 So in this thread, let us focus on how to fix options.test mid-term. I
 currently see 2 proposed ways:
 - Making the plugins aware of options.test
 - Make ldap2 write operations only print the update and not do it.
 Although thinking of this approach, I think it may make some plugins
 like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
- search for all unfixed DNS zones
- fix them with ldap update
- was the search truncated, i.e.  are there more zones to update?
  if yes, go back to start
 
  - Make the plugins not call {add,update,delete}_entry themselves but rather
 return the updates like they should. This is what the ticket
 (https://fedorahosted.org/freeipa/ticket/3448) requests and what should be
 done to make --test work for them.

How do you propose to handle iterative updates like the DNS upgrade mentioned
by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Jan Cholasta

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be
executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not
just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it
should be
fixed before committing.


Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We can
re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a
future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
log message
else
do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a
fix now
than a ticket that would never be done.


I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.

I like idea about mock_writes=True. However, I think we still need to
make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.


That is exactly why I mentioned LDAP transactions. There is no other
way how
to test complex plugins which actually read own writes (except mocking
the
whole LDAP interface somewhere).


While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test mid-term. I
currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
   - search for all unfixed DNS zones
   - fix them with ldap update
   - was the search truncated, i.e.  are there more zones to update?
 if yes, go back to start


 - Make the plugins not call {add,update,delete}_entry themselves but 
rather return the updates like they should. This is what the ticket 
(https://fedorahosted.org/freeipa/ticket/3448) requests and what 
should be done to make --test work for them.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Jan Cholasta

Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):

On 13.3.2015 12:01, Martin Basti wrote:

On 13/03/15 11:55, Petr Spacek wrote:

On 13.3.2015 11:34, Jan Cholasta wrote:

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be
executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.




Ideally we want to fix all plugins to dry-run the upgrade not
just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.


I agree that this breaks the spirit of --test and think it
should be
fixed before committing.

Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We can
re-visit it
later on, open a ticket :-)


No. There is no rush for this, at least not for the promise of a
future
fix that will never come.

I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
  log message
else
  do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a
fix now
than a ticket that would never be done.

I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.

I like idea about mock_writes=True. However, I think we still need to
make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.

That is exactly why I mentioned LDAP transactions. There is no other
way how
to test complex plugins which actually read own writes (except mocking
the
whole LDAP interface somewhere).

While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test mid-term. I
currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
 - search for all unfixed DNS zones
 - fix them with ldap update
 - was the search truncated, i.e.  are there more zones to update?
   if yes, go back to start

   - Make the plugins not call {add,update,delete}_entry themselves but rather
return the updates like they should. This is what the ticket
(https://fedorahosted.org/freeipa/ticket/3448) requests and what should be
done to make --test work for them.

How do you propose to handle iterative updates like the DNS upgrade mentioned
by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?


So instead of DNS commands logic, which can be used in plugin, we should

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Martin Basti

On 13/03/15 12:50, Jan Cholasta wrote:

Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):

On 13.3.2015 12:01, Martin Basti wrote:

On 13/03/15 11:55, Petr Spacek wrote:

On 13.3.2015 11:34, Jan Cholasta wrote:

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:
Upgrade plugins which modify LDAP data directly should 
not be

executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.



Ideally we want to fix all plugins to dry-run the upgrade 
not

just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.


I agree that this breaks the spirit of --test and think it
should be
fixed before committing.

Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We 
can

re-visit it
later on, open a ticket :-)

No. There is no rush for this, at least not for the promise 
of a

future
fix that will never come.
I checked the code and to me, the proper fix looks like 
instrumenting

ldap.update_entry calls in upgrade plugins with

if options.test:
  log message
else
  do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry 
ipaserver/install/plugins

ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also 
prefer a

fix now
than a ticket that would never be done.

I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to 
add if

otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.
I like idea about mock_writes=True. However, I think we still 
need to

make
sure plugin writers rely on options.test value to see that we 
aren't
going to write the data. The reason for it is that we might get 
into
configurations where plugins would be doing updates based on 
earlier
performed tasks.  If task configuration is not going to be 
written, its

status will never be correct and plugin would get an error.
That is exactly why I mentioned LDAP transactions. There is no 
other

way how
to test complex plugins which actually read own writes (except 
mocking

the
whole LDAP interface somewhere).

While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test 
mid-term. I

currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
 - search for all unfixed DNS zones
 - fix them with ldap update
 - was the search truncated, i.e.  are there more zones to 
update?

   if yes, go back to start
   - Make the plugins not call {add,update,delete}_entry 
themselves but rather

return the updates like they should. This is what the ticket
(https://fedorahosted.org/freeipa/ticket/3448) requests and what 
should be

done to make --test work for them.
How do you propose to handle iterative updates like the DNS upgrade 
mentioned

by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?

So 

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Martin Basti

On 13/03/15 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:
Upgrade plugins which modify LDAP data directly should not be 
executed

in --test mode.

This patch is a workaround, to ensure update with --test option 
will not

modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just 
skip

when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that 
this
requires 'proper' fix now. It was broken for *years* so this patch 
is a huge
improvement and IMHO should be commited in current form. We can 
re-visit it

later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting 
ldap.update_entry calls in upgrade plugins with


if options.test:
   log message
else
   do the update

right? I see just couple places that would need to be updated:

$ git grep -E (ldap|conn).update_entry ipaserver/install/plugins
ipaserver/install/plugins/dns.py: ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)

ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_pacs.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py: 
ldap.update_entry(uid_uniqueness_plugin)



So from my POV, very quick fix. In that case, I would also prefer a 
fix now than a ticket that would never be done.


Martin

And ldap.add_entry, del_entry, ipa add/mod/del commands

--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-13 Thread Rob Crittenden
Jan Cholasta wrote:
 Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):
 On 13.3.2015 12:01, Martin Basti wrote:
 On 13/03/15 11:55, Petr Spacek wrote:
 On 13.3.2015 11:34, Jan Cholasta wrote:
 Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):
 On 03/13/2015 11:00 AM, Petr Spacek wrote:
 On 13.3.2015 10:42, Alexander Bokovoy wrote:
 On Fri, 13 Mar 2015, Petr Spacek wrote:
 On 13.3.2015 10:18, Martin Kosek wrote:
 On 03/12/2015 05:10 PM, Rob Crittenden wrote:
 Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should
 not be
 executed
 in --test mode.

 This patch is a workaround, to ensure update with --test
 option will not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.



 Ideally we want to fix all plugins to dry-run the upgrade not
 just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.

 I agree that this breaks the spirit of --test and think it
 should be
 fixed before committing.
 Considering how often is the option is used ... I do not think
 that this
 requires 'proper' fix now. It was broken for *years* so this
 patch is a huge
 improvement and IMHO should be commited in current form. We can
 re-visit it
 later on, open a ticket :-)

 No. There is no rush for this, at least not for the promise of a
 future
 fix that will never come.
 I checked the code and to me, the proper fix looks like
 instrumenting
 ldap.update_entry calls in upgrade plugins with

 if options.test:
   log message
 else
   do the update

 right? I see just couple places that would need to be updated:

 $ git grep -E (ldap|conn).update_entry
 ipaserver/install/plugins
 ipaserver/install/plugins/dns.py:
 ldap.update_entry(container_entry)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/update_idranges.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_idranges.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(base_entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_pacs.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_referint.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_services.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_uniqueness.py:
 ldap.update_entry(uid_uniqueness_plugin)


 So from my POV, very quick fix. In that case, I would also
 prefer a
 fix now
 than a ticket that would never be done.
 I really dislike this approach because I consider it flawed by
 design. Plugin
 author has to think about it all the time and do not forget to
 add if
 otherwise ... too bad.

 I can see two 'safer' ways to do that:
 - LDAP transactions :-)
 - 'mock_writes=True' option in LDAP backend which would print
 modlists instead
 of applying them (and return success to the caller).

 Both cases eliminate the need to scatter 'ifs' all over update
 plugins and do
 not add risk of forgetting about one of them when adding/changing
 plugin code.
 I like idea about mock_writes=True. However, I think we still
 need to
 make
 sure plugin writers rely on options.test value to see that we
 aren't
 going to write the data. The reason for it is that we might get
 into
 configurations where plugins would be doing updates based on
 earlier
 performed tasks.  If task configuration is not going to be
 written, its
 status will never be correct and plugin would get an error.
 That is exactly why I mentioned LDAP transactions. There is no other
 way how
 to test complex plugins which actually read own writes (except
 mocking
 the
 whole LDAP interface somewhere).
 While this may be a good idea long term, I do not think any of us is
 considering implementing the LDAP transaction support within work on
 this refactoring.

 So in this thread, let us focus on how to fix options.test
 mid-term. I
 currently see 2 proposed ways:
 - Making the plugins aware of options.test
 - Make ldap2 write operations only print the update and not do it.
 Although thinking of this approach, I think it may make some plugins
 like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
  - search for all unfixed DNS zones
  - fix them with ldap update
  - was the search truncated, i.e.  are there more zones to
 update?
if yes, go back to start
- Make the plugins not call {add,update,delete}_entry themselves
 but rather
 return the updates like they should. This is what the ticket
 (https://fedorahosted.org/freeipa/ticket/3448) requests and what
 should be
 done to make --test work for them.
 How do you propose to handle iterative updates like the DNS upgrade
 mentioned
 by Martin^1? Return set of updates along 

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-12 Thread David Kupka

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be executed
in --test mode.

This patch is a workaround, to ensure update with --test option will not
modify any LDAP data.

https://fedorahosted.org/freeipa/ticket/3448

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just skip 
when there is '--test' option but it is a good first step.

Works for me, ACK.

--
David Kupka

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-12 Thread Petr Spacek
On 12.3.2015 17:05, Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be executed
 in --test mode.

 This patch is a workaround, to ensure update with --test option will not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.




 Ideally we want to fix all plugins to dry-run the upgrade not just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.


 I agree that this breaks the spirit of --test and think it should be
 fixed before committing.
 
 Considering how often is the option is used ... I do not think that this
 requires 'proper' fix now. It was broken for *years* so this patch is a huge
 improvement and IMHO should be commited in current form. We can re-visit it
 later on, open a ticket :-)

BTW 'proper' fix would be to implement transactions in DS, run all updates in
one huge transaction and do roll-back at the end. That would allow us to
actually test updates which can depend on each other etc.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-12 Thread Rob Crittenden
David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be executed
 in --test mode.

 This patch is a workaround, to ensure update with --test option will not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.



 
 Ideally we want to fix all plugins to dry-run the upgrade not just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.
 

I agree that this breaks the spirit of --test and think it should be
fixed before committing.

rob

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-12 Thread Rob Crittenden
Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be executed
 in --test mode.

 This patch is a workaround, to ensure update with --test option will not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.




 Ideally we want to fix all plugins to dry-run the upgrade not just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.


 I agree that this breaks the spirit of --test and think it should be
 fixed before committing.
 
 Considering how often is the option is used ... I do not think that this
 requires 'proper' fix now. It was broken for *years* so this patch is a huge
 improvement and IMHO should be commited in current form. We can re-visit it
 later on, open a ticket :-)
 

No. There is no rush for this, at least not for the promise of a future
fix that will never come.

rob

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

2015-03-12 Thread Petr Spacek
On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be executed
 in --test mode.

 This patch is a workaround, to ensure update with --test option will not
 modify any LDAP data.

 https://fedorahosted.org/freeipa/ticket/3448

 Patch attached.




 Ideally we want to fix all plugins to dry-run the upgrade not just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.

 
 I agree that this breaks the spirit of --test and think it should be
 fixed before committing.

Considering how often is the option is used ... I do not think that this
requires 'proper' fix now. It was broken for *years* so this patch is a huge
improvement and IMHO should be commited in current form. We can re-visit it
later on, open a ticket :-)

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code