Petr Viktorin wrote:
On 03/30/2012 03:22 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/16/2012 08:01 PM, Petr Viktorin wrote:
On 03/16/2012 06:35 PM, Petr Viktorin wrote:
On 03/16/2012 06:33 PM, Rob Crittenden wrote:
Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/15/2012 09:24 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/29/2012 04:34 PM, Petr Viktorin wrote:
On 02/29/2012 03:50 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/27/2012 11:03 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
Patch 16 defers validation & conversion until after
{add,del,set}attr is
processed, so that we don't search for an integer in a
list of
(this caused ticket #2405), and so that the end result of
operations is validated (#2407).

Patch 17 makes these options honor params marked no_create

NACK on Patch 17 which breaks patch 16.

How is patch 16 broken? It works for me.

My point is they rely on one another, IMHO, so without 17 the
problem still exists.

*attr is specifically made to be powerful. We don't want to
block updating certain values.


Not having patch 17 means that the problem reported in 2408
occurs. It should probably check both the schema and the
param to
determine if something can have multiple values and reject

I see the problem now: the certificate subject base is defined
as a
multi-value attribute in the LDAP schema. If it's changed to
single-value the existing validation should catch it.

Also, most of the config attributes should probably be
required in
schema. Am I right?

I'm a newbie here; what do I need to do when changing the
schema? Is
there a patch that does something similar I could use as an

The framework should be able to impose its own single-value
will as
well. If a Param is designated as single-value the *attr should

Here is the updated patch.
Since *attr is powerful enough to modify 'no_update' Params,
CRUDUpdate forgets about, I need to check the params of the

Updating schema is a bit of a nasty business right now. See
10-selinuxusermap.update for an example.

I'll leave schema changes for after the release, then.

Freeipa-devel mailing list

Attached patch includes tests. Note that the test depends on my
12-13, which make ipasearchrecordslimit required.

I gather that this eliminates the need for patch 17? It seems to

Yes. Patch 17 made *attr honor no_create and no_update, which you
is not desired behavior.

The patch doesn't apply because of an encoding change Martin made

It does seem to do the right thing though.


Attaching rebased patch.
This deletes Martin's change, but unless I tested wrong, his bug
( stays fixed. The
tests in
my patch should apply to that ticket as well.

In another fork of this thread, there's discussion if this
approach is
good at all. Maybe we're overengineering a corner case here.

Found another issue, a very subtle one.

When using *attr and an exception occurs where the param name would
appear we want the name passed in to be used.

For example:

$ ipa pwpolicy-mod --setattr=krbpwdmaxfailure=xyz

With this patch it will return:
ipa: ERROR: invalid 'maxfail': must be an integer

It should return:
ipa: ERROR: invalid 'krbpwdmaxfailure': must be an integer

On second thought, this may not be related...

This is, I'll see if it
makes sense to fix it here.

Okay, this is a different problem. A quick grep of ConversionError in reveals that all of the "wrong datatype" errors are
with the raw parameter name.
Other errors are raised with either that or the cli_name or they
auto-detect. I don't think they follow some rule in this.

Furthermore, our test suite doesn't check exception arguments. Sounds
like a major cleanup coming up here...

The encoding problem does still exist too:

$ ipa config-mod --setattr ipamigrationenabled=false
ipa: ERROR: ipaMigrationEnabled: value #0 invalid per syntax:

Will fix.

Fixed in the attached update, which encodes the value.

I was surprised to find that
config_mod.params['ipamigrationenabled'].attribute is True, while
config_mod.obj.params['ipamigrationenabled'].attribute is False (and so
its encode() method does nothing). That's because 'attribute' is only
set when the params are cloned from the LDAPObject to the CRUD method.
Is there a reason behind this, or is it just that it was easier to do?

For this case it means that params marked no_update will not be encoded
properly -- getting to a working encode() would require either setting
'attribute' on the parent object or some ugly hackery.


Rebased to current master.

There is a test failure. I ran out of time last night to investigate
whether the test is valid or it is an issue with the patch, forgot to
send this out before I left for the day yesterday.

FAIL: test_attr[23]: pwpolicy_mod: Try setting non-numeric
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose/", line 187, in
line 247, in <lambda>
func = lambda: self.check(nice, **test)
line 260, in check
self.check_exception(nice, cmd, args, options, expected)
line 277, in check_exception
UNEXPECTED % (cmd, name, args, options, e.__class__.__name__, e)
AssertionError: Expected 'pwpolicy_mod' to raise ConversionError, but
caught different.
args = []
options = {'setattr': u'krbpwdmaxfailure=abc'}
ValidationError: invalid 'krbpwdmaxfailure': must be an integer

Fixed. I should have caught this on Friday.

ACK, pushed to master and ipa-2-2


Freeipa-devel mailing list

Reply via email to