On 25/05/16 16:07, Jan Cholasta wrote:
On 25.5.2016 15:03, David Kupka wrote:
On 18/05/16 08:01, Jan Cholasta wrote:
On 16.5.2016 16:35, Martin Basti wrote:


On 09.05.2016 14:07, Jan Cholasta wrote:
On 6.5.2016 14:32, Martin Basti wrote:


On 28.04.2016 14:45, Jan Cholasta wrote:
Hi,

I have pushed my thin client WIP branch to GitHub:
<https://github.com/jcholast/freeipa/tree/trac-4739>.

All commits up to "ipalib: use relative imports for cross-plugin
imports" should be good for review. The rest is subject to change
(WARNING: I will force push into this branch).

Honza

I did not test it yet, I just checked the code

* automount: do not inherit automountlocation_tofiles from
LDAPQuery *
LGTM

* dns: move all dnsrecord code called on client to a single class *
LGTM

* dns: do not rely on server data structures in code called on
client *
1)
you forgot to increment VERSION

This was deliberate, as it will no longer be necessary to bump VERSION
for backward compatible changes after this whole patchset is merged.
But we're not there yet, so fixed.

How we should handle VERSION after your patches?

Otherwise LGTM

* otptoken: fix import of DN *
ACK

* otptoken_yubikey: fix otptoken_add_yubikey arguments *
1)
you forgot to increment VERSION

Fixed.


2)
Did you find out why this was issue?
-        del answer['value']    # Why does this cause an error if
omitted?
 -        del answer['summary']  # Why does this cause an error if
omitted?

The command definition was not complete, it was missing has_output.


Otherwise LGTM

* vault: move client-side code to the module level *
LGTM

* vault: copy arguments of client commands from server counterparts *
1)
you forgot to increment Version

Fixed.


* ipalib: use relative imports for cross-plugin imports *
1) Missing explanation for future generations why this change is
needed
in commit message

Fixed.


The other commits I will check later.
Martin^2


Okay. Thanks.


* frontend: remove the unused Command.soft_validate method
LGTM

* frontend: perform argument value validation only on server
LGTM

* frontend: do not forward argument defaults to server
I'm not a fan of returning  None in promt_param function, but I havent
found anything better to use.

It always returned None for unset params.

LGTM

* ipalib: optimize API.txt
this contains a lot of black magic, but because this is mainly copy of
current to proper places, LGTM

It's actually mostly cut-n-paste.


* makeaci: load additional plugins using API.add_module
Looks good, but I miss explanation why is this change needed

The next change would be impossible without this.


* plugable: replace API.import_plugins with new API.add_package
LGTM


* ipalib, ipaserver: migrate all plugins to Registry-based registration
ACK

* ipalib, ipaserver: fix incorrect API.register calls in docstrings
LGTM


* plugable: remove the unused deprecated API.register method
LGTM


* plugable: switch API to Registry-based plugin discovery
LGTM

* frontend: merge baseldap.CallbackRegistry into Command
LGTM

*frontend: move the interactive_prompt callback type to Command
 LGTM

Martin^2





Hello,
first batch of 30 patches from Honza's trac-4739 branch
(https://github.com/jcholast/freeipa/commits/trac-4739) is ready to be
pushed into master.
All upto "frontend: allow commands to have an argument named `name`" got
over numerous test&fix cycles in last week+ and is working as expected
now, ACK.

Thanks for the review.


Honzo, please rebase and push them, thanks!


Attaching the patches for reference.

Pushed to master: 2b50fc617024f18a81e97f30f75ed1b9221c4055


Hello,
another patchset is ready. There are still some minor issues with interactive prompt in dns* commands but these can be fixed later. Otherwise all work as expected, ACK.

Also it would be good to have tests for schema plugin, I have filled a ticket [1].

List of commits in Honza's trac-4739 branch:
frontend: do not check API minor version of the client
ipalib: move server-side plugins to ipaserver
ipaclient: implement thin client
misc: hide the unused --all option of `env` and `plugins` in CLI
ipalib: move File command arguments to ipaclient
ipactl: use server API
client install: finalize API after CA certs are available
rpc: do not validate command name in RPCClient.forward
rpc: optimize JSON-RPC response handling
rpc: specify connection options in API config
rpc: allow overriding NSS DB directory in API config
rpc: respect API config in RPCClient.create_connection
ipalib: introduce API schema plugins
ipalib: replace DeprecatedParam with `deprecated` Param argument
parameters: introduce no_convert keyword argument
parameters: introduce cli_metavar keyword argument
ipalib: split off client-side plugin code into ipaclient
dns: move code shared by client and server to separate module
ipaclient: add client-side command override class
frontend: turn Method attributes into properties
plugable: remember overriden plugins in API
plugable: simplify API plugin initialization code
plugable: turn Plugin attributes into properties
help, makeapi: do not use hardcoded plugin package name
help, makeapi: specify module topic by name
help, makeapi: allow setting command topic explicitly
ipalib: move client-side plugins to ipaclient
ipaclient: introduce ipaclient.plugins
dns: fix dnsrecord interactive mode
cli: make optional positional command arguments actually optional

[1] https://fedorahosted.org/freeipa/ticket/5929
--
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

Reply via email to