Certificate mapping test #399
Conversation
There are multiple types of entries and objects accessible in API and not all of them have the same set methods. Spliting Tracker into multiple trackers should reflect this better.
ipatests/test_xmlrpc/tracker/base.py
Outdated
| raise NotImplementedError(self._override_me_msg) | ||
|
|
||
| class Tracker(RetrievableTracker, SearchableTracker, MutableTracker, | ||
| CreatableTracker): |
There was a problem hiding this comment.
Please fill in the docstrings for the parent classes and describe their intended usage there.
ipatests/test_xmlrpc/tracker/base.py
Outdated
|
|
||
| request.addfinalizer(cleanup) | ||
|
|
||
| return super(CreatableTracker, self).make_fixture(request) | ||
| raise NotImplementedError(self._override_me_msg) |
There was a problem hiding this comment.
Please remove the leftover raise statement
| if got: | ||
| assert_deepequal(expected, got) | ||
| else: | ||
| assert("Command didn't returned") |
There was a problem hiding this comment.
Shouldn't this line read assert False, "Command didn't return"?
| (CREATE_PERM, errors.ACIError, None,), | ||
| (READ_PERM, errors.NotFound, None,), | ||
| ], | ||
| ): |
There was a problem hiding this comment.
There is a lot of duplication of the same parameters passed to the execute_with_expected. Please hide common code into a separate private method or use functools.partial.
| try: | ||
| certmap_rule.delete() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
this is also copy-pasted a lot so it warrants a separate method for that.
|
|
||
| def test_update(self, certmap_rule, certmap_user_permissions): | ||
| certmap_rule.ensure_missing() | ||
| certmap_rule.ensure_exists() |
There was a problem hiding this comment.
is there a reason why some of the tests use ensure_missing followed by ensure_exists and some just use ensure_exists?
|
I have some inline comments. I was also thinking about the nomenclature of the Tracker mixins and I think we should name them based on the noun of the action that is being tracked, e.g: RetrievalTracker |
| u'nsContainer', | ||
| u'ipaCertMapContainer', | ||
| u'ipaCertMapConfigObject', | ||
| ] |
There was a problem hiding this comment.
The schema was simplified and will only contain top, nsContainer and ipacertmapconfigobject
| u'consumed by SSSD', | ||
| u'ipacertmapmatchrule': u'arbitrary free-form matching rule defined ' | ||
| u'and consumed by SSSD', | ||
| u'associateddomain': u'example.org', |
There was a problem hiding this comment.
certmaprule-add has a check on the 'associateddomain' attribute. It must either be FreeIPA domain or a trusted domain. Because of this check, the creation with this constant is likely to fail.
| u'ipacertmapissuer': DN('CN=Changed CA,O=OTHER.ORG'), | ||
| u'ipacertmapmaprule': u'changed arbitrary mapping rule', | ||
| u'ipacertmapmatchrule': u'changed arbitrary maching rule', | ||
| u'associateddomain': u'changed.example.org', |
There was a problem hiding this comment.
Same comment as above: associateddomain must either be FreeIPA domain or a trusted domain.
| u'cn': u'test_rule', | ||
| u'description': u'Certificate mapping and matching rule for test ' | ||
| u'purposes', | ||
| u'ipacertmapissuer': DN('CN=CA,O=EXAMPLE.ORG'), |
There was a problem hiding this comment.
The issuer attribute has been removed from the original design
|
|
||
| certmaprule_update_params = { | ||
| u'description': u'Changed description', | ||
| u'ipacertmapissuer': DN('CN=Changed CA,O=OTHER.ORG'), |
There was a problem hiding this comment.
The issuer has been removed from the original design.
| u'dn', | ||
| u'cn', | ||
| u'description', | ||
| u'ipacertmapissuer', |
There was a problem hiding this comment.
you can remove ipacertmapissuer attribute
| self.api.env.basedn) | ||
| self.options = { | ||
| u'description': description, | ||
| u'ipacertmapissuer': ipacertmapissuer, |
There was a problem hiding this comment.
you can remove ipacertmapissuer attribute.
| u'objectclass', | ||
| u'aci', | ||
| u'ipacertmapversion', | ||
| u'ipaconfigstring' |
There was a problem hiding this comment.
you can remove ipacertmapversion and ipaconfigstring
| u'cn': [self.api.env.container_certmap[0].value], | ||
| u'objectclass': objectclasses.certmapconfig, | ||
| u'ipacertmapversion': [u'1'], | ||
| u'ipaconfigstring': [u'CertMapVersion 1'], |
There was a problem hiding this comment.
you can remove ipacertmapversion and ipaconfigstring
| ): | ||
| raise MutuallyExclusiveError(reason=u'Mutually exclusive options ' | ||
| u'provided at the same time.') | ||
|
|
There was a problem hiding this comment.
You can also add a requirement: if subject or issuer is provided, then the other one is also required.
|
This's been hanging here for long enough, why isn't it finished? I don't see any postponed label here, do we know whether it's somehow blocked? |
ghost
mentioned this pull request
https://fedorahosted.org/freeipa/ticket/6542