#48360 Refactor the delete agreement function and add a test for it
Closed: wontfix None Opened 8 years ago by spichugi.

The delete agreement function of agreement.py module doesn't cover all usage varieties and should be rewritten.

Now it requires suffix and replica(DirSrv object of the server that the agreement points to). By some reasons, this replica object may be missing, then we couldn't delete the agreement.

I propose to change arguments of the delete agreement function to:

  • suffix
  • consumer_host - of the server that the agreement points to
  • consumer_port - of the server that the agreement points to

With that, we can clearly define the required agreement.

If anybody has some additional ideas, please share them.

Also, we don't have test coverage for that function. It should be added to tests/agreement_test.py.


I'm happy with the code here for the deletion: I think this is much more specific and a good improvement.

given that the cn of a replication agreement has to be unique also, could we also delete by agreement name?

If you want to add deletion by agreement name, I'd be happy with that, if not your current patch has my ack.

William, sure, no problem.

Feature was added, please, test and review.

The new feature looks good!

With the test case, because we only delete one test aren't we only testing the agmtdn delete code path? We need two tests, one for agmtdn, and one for consumer_host / port. Sorry to be a pain!

Replying to [comment:4 firstyear]:

The new feature looks good!

With the test case, because we only delete one test aren't we only testing the agmtdn delete code path? We need two tests, one for agmtdn, and one for consumer_host / port. Sorry to be a pain!

No problem. Thank you. :)

For now, I have other tasks with a higher priority. I've added a "TODO" block, so I can come back to it when the time will come.

{{{
187 # TODO: add a few test cases for different sets of arguments
188 # using fixtures and parameters.
}}}

Just so long as you do come back to it! :)

Ack from me.

To ssh://git.fedorahosted.org/git/389/lib389.git

252e0c1..0b9729b master -> master
commit 0b9729bacf5a8a6529559d81044fbb5973cf9b84
Author: Simon Pichugin spichugi@redhat.com
Date: Thu Nov 26 16:55:52 2015 +0100

Milestone lib389 1.0 deleted

Metadata Update from @spichugi:
- Issue assigned to spichugi

7 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1691

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

3 years ago

Login to comment on this ticket.

Metadata