On 25.8.2015 17:50, Simo Sorce wrote:
On Tue, 2015-08-25 at 16:22 +0200, Jan Cholasta wrote:
On 25.8.2015 16:09, Simo Sorce wrote:
On Tue, 2015-08-25 at 15:52 +0200, Jan Cholasta wrote:
On 25.8.2015 15:37, Simo Sorce wrote:
On Tue, 2015-08-25 at 11:34 +0200, Jan Cholasta wrote:
On 25.8.2015 11:25, Jan Cholasta wrote:
On 24.8.2015 19:51, Simo Sorce wrote:
Why do we have cainstance.py and ca.py and krainstance.py and kra.py in
ipaserver/install when you always need both files to do anything around
installation of the ca ?

Is there a motivation ?
Or can I simply provide a patch to remove the ca.py and kra.py files an
unify all code in the proper *instance.py file ?

ca.py and kra.py are the proper files ready to be migrated to the new
install framework, cainstance.py and krainstance.py will be removed.

... once the migration is done. (Hit send button too fast.)

The motivation is that *instance.py do not provide a uniform interface,
have a lot of redundant and duplicate stuff and are generally unfit for
any further extension.

I have been changing only the instance files, so we are going in
different directions.

I don't see how, {ca,kra}instance.py code is called from {ca,kra}.py.
{ca,kra}.py contains all the code that is actually needed to install
CA/KRA that is not in {ca,kra}instance.py and was previously scattered
around ipa-{server,replica,ca,kra}-install.


I do not really care what file we are going into, but there is a lot of
code in the installer now that does not tell the user a step is being
done, while instances do that through the step interface.

This code was always there, we only moved it to a single location. See
git history.


The step interface is also a very good way to let someone that read the
code see what is going on and follow each step.

Are you proposing to stop going through the instances steps ? I found
the current way kra and ca installation is setup basically a regression,
it took me a *lot* longer that it should be needed to follow through all
the steps that are really taken.

Again, we only moved the code around, and I don't think we created any
regressions.

Ok, so the plan is just to move the CAInstance class and code *as is*
from cainstance.py to ca.py ?I guess I am confused about what is your
plan around this exactly.

The code in CAInstance will be gradually migrated to a new class in
ca.py which uses the new install framework. We started with the
top-level ipa-server-install and ipa-replica-install code, which is
still not done, but you can see it for reference on how it will look
like (ipaserver/install/server/*.py, especially the classes at the
bottom of the files).

Well I had to modify server/replicainstall.py quite radically, I had to
create a new set of promote_check and promote functions there. So we are
back to duplicated code, sorry (no way around it).

It's OK, there is still a lot of duplicate code in server/replica install anyway, we deduplicated only the CA, KRA and DNS install code so far.


However the functions in server/replicainstall.py still use the
instances mostly for all components *except* for kra and ca where there
is really confusing code calling unnecessarily instances multiple times
and fooling around with stuff.

If you think there are unnecessary calls I think you are reading the code wrong.

I do not really understand what you mean
by migrating from CAInstance to what's in server/*install.py because in
there all instances are used for the individual components, so I am now
scratching my head.

What I mean is that the code to install a component will be wrapped in a class similar to Server or Replica classes in these files. There will still be steps like in CAInstance, but the rest will be different. I don't think you have seen the PoC from 5 months ago: <https://www.redhat.com/archives/freeipa-devel/2015-March/msg00374.html>.


The code in ca.py especially is really confusing, I rewrote it once to
eliminate duplications then decided to simply not touch it in my branch
(and threw away the modifications) because it is too confusing and I did
not want to risk regressions. So I created a simple create_replica()
function in the CA instance that does all that is needed.

Before ca.py, you would have to rewrite the same confusing code in ipa-server-install, ipa-replica-install and ipa-ca-install. I don't see how that's better.


I guess I will just keep ignoring the confusion and try to come up with
something that "works" but I'd really like to understand what is the
endgame there. If you want to replace instances (why?), what will you
replace it with ?

With something that does not mix install with service management, maximizes code reuse and makes related code colocated, provides introspection on configuration options, allows componentization and idempotent execution. See the PoC linked above.


Simo.



--
Jan Cholasta

--
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