> On Jul 14, 2015, at 3:41 AM, Owen Synge <osynge@xxxxxxxx> wrote: > > Dear Travis, > > We clearly disagree in this area. > > I hope me explaining my perspective is not seen as unhelpful. > > On 07/09/2015 07:00 PM, Travis Rhoden wrote: >>> (2B) inflexible / complex include paths for shared code between facade >>>> implementations. >>>> >> I disagree here. There are plenty of places to put code that is common and does not have to be duplicated. I definitely don’t want code in each host module that knows how to deal with systemd. The code that does have to be in each host module is how to detect/assign what init system is being used, and how to call into the appropriate (common) module to do what it needs. >> >> As for complex include paths, I’m not sure I have much of an argument here other than to say that all frameworks have their conventions. I don’t see what we are doing as anything different than what is enforced/recommended by many frameworks/scaffolds popular in Web app and MVC paradigms like RoR, Pyramid, Django, etc. It’s just convention to learn. > > I don't understand why you chose to defend this. Please expand your > argument. Truthfully I’m not sure how to, because I’m not sure I agree that code location is enforced at all. I don’t think that it is. There is nothing “enforcing” where code lives. By that I mean that as long is once class/function/module is importable into another module, it works. There is a general style that has fallen out as development has happened over time, but like most projects there are bits and pieces scattered throughout in places that probably made logical sense at the time, but no longer is the best fit. I say nothing is “enforcing” locations because it’s not like the code won’t work if the code is some place else. As long you can import what you need, things should be good. There used to be relative imports in ceph_deploy (e.g. “import ..utils; import .helpers”), but I believe all of that has been gone for a while. For the most part it’s really up to the author of new code to put it where they think is best, and for the reviewers to agree or be indifferent about it. :) Along those lines, if code were to get moved in a PR with a description of “this makes a lot more sense”, that would be seen as a positive contribution. Most of the “rigid” layout is in the hosts modules. It makes sense that we have hosts/centos, hosts/debian, etc. But then even inside to those top levels, they import things from all over the place. One example I can think of is ceph_deploy.hosts.remotes, the contents of which magically (except, not magic) appear as distro.conn.remote_module. This is using a capability of python-remoto, and is an area of code I’ve spent very little time with it. I’m not sure if this is perhaps what you are concerned about. > > I will not go into the negative side effects of importing modules as > objects in the include paths, as a work around for potential side > effects of our current façade model, as this topic is more complex than > the points I do raise in this email, but I will expand them should these > arguments not be valid, as I would like to understand where we intend to > go, and how we minimise long term maintenance and maximise extensibility. > > Here are the simpler reasons why I don't agree: > > I hate the enforced code structures used my many "popular" frameworks > especially common in web frameworks but I accept that these enforce very > clear standard design patterns that are known to scale and be extensible > and you acknowledge this difference to the ceph-deploy code base simply > by calling them MVC. * > > Having enforced relative paths between code files defined by the > implementation of the code is a great limitation. > > While one can place code modules in the correct location if only one > thing defines the correct location, one can easily get into difficulties > 2 obvious limitations in tolerating this are: > > (1) by having two parts of the code base enforcing relative paths > between modules. I hope that I’ve made it clear that I don’t think anything is actually enforcing any relative paths between modules. That doesn’t mean I’m not missing something, so do please point it out if I am. > > (2) by having relationships between modules become needlessly complex > because the enforced relationship says the code must go in a place that > is not correct to solve the problem. (this is analogous to the death of > hierarchical databases and their replacement with relational databases) > > So since the issue is most serious if you have 2 components in the code > base that "enforce" module layout, and we only have one, we are only > preventing further components that "enforce" module layout. > > Admittedly neither of these issues has _prevented_ development on > ceph-deploy so far, or led to duplication now you withdrew your comments > about all façades being the same. > > Due to the façade defining module location we so far have, some things > have become confused in where the module can be placed that are clearly > OS specific. > > Examples include: > > ceph_deploy/util/constants.py which is clearly includes constants that > are OS specific, such as package names. I get your point, but really these could be anywhere. For all the OS’s we’ve been supporting, most of this is *not* OS specific, really. They are indeed constants across all Linux distros. There’s been new additions to deal with downstream package splits, and there very well be better ways of handling that. Those pieces are relatively new (just a few months), and I highly suspect that will be improved more over time. I also fully accept that while perhaps these were simple/universal contstants in the past, the variety of packaging we support, distros, and init systems may not make that the case (systemd comes to mind). However it is important to keep in mind that ceph-deploy is only intended to support installing Ceph from upstream Ceph release/testing packages, or downstream packages that follow the same locations about where to install things to, not homegrown or locally compiled binaries. ceph-deploy is purposely a highly opinionated tool. > > ceph_deploy/util/pkg_managers.py which clearly is a set of OS specific > implementations. Not so much OS specific, as packager specific (yum, apt, etc.) This area is ripe for improvement, and is actually something I’ve started on already in order to support DNF on Fedora. I have a branch I can point you at if interested. But I want to make sure I truly understand your concern here. If there were classes in ceph_deploy.util.pkg_managers, one for each type of of manager we use (dnf, yum, apt, zypper), and these were imported as needed by host modules in ceph_deploy.hosts.*, would you see that as a problem? It doesn’t matter to me where the code is. It’s more important to me that the module names are clear, so I don’t have to find package manager code in a file named “decorators” or something. > > Both of these lead to unnecessary complications, when they could be > façades in their own right and should have the modules moved if we > continue to have this enforced module location in the façade implementation. Totally agree that it can be improved. ceph-deploy is growing organically and has had a lot of different developers on as you would expect from any project of this nature. > > This inconsistency will not effect customers but does effect the clarity > of the code, and leads to more conditionals than we might have if we > took in my opinion a cleaner implementation to our model. > > I will _not_ provide sample code of improvements, as this may lead to > people discussing irrelevant issues, until this point is resolved as my > mistake, or an area that is agreed can be improved. > > Best regards > > Owen > > > * It is one the many reasons I abandoned django in favour of Pyramid for > my personal projects, even though django only enforced separation of > components Model View and controller, pyramid to my recollection (its > been 3 years at least) only recommended a module relationship path . I rather enjoyed working with Pyramid as well, and it’s precursor pylons.-- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
