Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

New commands (including sync with file, validity management…) #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cyrilst
Copy link

@cyrilst cyrilst commented Jun 11, 2020

Hello,

as promised in #39 here is a PR containing many changes.

I have some comments about these changes (in addition to the ones already discussed in #39):

  • please have a look at _decode_all, as I'm not sure it's the right way to do it (first time I deal with decoding).
  • for the delete wildcard, I just added an or pattern == '*' condition, certainly the best approach if we want to keep it simple.
  • the add command now accepts more than one key in the file.
  • for the add command, I added the --update option which updates the expiration date of the keys if they are already existing.
  • please have a look at sync_pubkeys. I use print in this function, which is not a thing we want in __init__.py. But on the other hand, I have to print near private methods… Maybe should I append the output to a variable and return this variable, but it's not really good either, because some output could be missed in the case of an error raise. I'll take any good idea for this, I guess I'll have to refactor this method.
  • I'm not fond of a unique --help containing all the options. As a user, I really prefer when there are specific helps for each command, containing just the appropriate options. But on the other hand, doing this means the tool is now a bit more complex than before…
  • and finally, should I add some tests ? Maybe use docker to create an ldap server, and check if the different command / options give the expected results…

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant