[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Apache Fineract CN API Documentation

Hello everyone,

I've made some changes to the approach for API documentation based on
Myrle's feedback.

So I have reverted my recent changes to fineract-cn-customer and created a
pull request [1] for review. I am hoping that we'll get all necessary fixes
on the PR before I move on to documenting the other services.

I have also documented the process [2] for API Documentation which
stipulates how a developer can generate the snippets for documentation ( in
component-test's build/doc/ folder ) given that we don't want .adoc file
and such checked into the repository.

I await your thoughts on this.

[1] https://github.com/apache/fineract-cn-customer/pull/7

On Wed, Apr 25, 2018 at 6:09 PM, Isaac Kamga <isaac.kamga@xxxxxxxxx> wrote:

> Hello Myrle,
> I've done some tinkering and Spring REST Docs can indeed get the snippets
> generated for documentation in the component-test module. At least, that
> ensures that there's no need for an architectural change and the service
> module doesn't depend on the component-test module as Markus advised.
> I think there are still other concerns which you highlighted;
> 1. Checking in generated documentation : Should we leave them in
> component-test's build folder (which is ignored by git) and rather document
> the process of generating the snippets ?
> 2. Creating a documentation module : Do we still need to try this out ?
> This module will import the service and component-test modules.
> 3. MockMvc vs Spring Fox: Were you concerned about the security of MockMvc
> ?
> I'd like to have this discussed here so that we don't proceed with
> approaches to solving the problem which aren't worthwhile. I had earlier
> assumed that a mention on the Quarter 1 board report about my work on API
> documentation would have sufficed.
> If you're okay with the API documentation occurring in the component-test
> module, it may be okay for me to revert the commits I made to the
> repositories and submit a Pull request which you'll review and merge.
> At Your Service,
> Isaac Kamga.
> On Wed, Apr 25, 2018 at 11:53 AM, Isaac Kamga <isaac.kamga@xxxxxxxxx>
> wrote:
>> Hi Myrle,
>> Thanks for your elaborate feedback and corrections.
>> On Wed, Apr 25, 2018 at 9:10 AM, Myrle Krantz <myrle@xxxxxxxxxx> wrote:
>>> Hey Isaac,
>>> I've looked at your changes in customer and I've found several
>>> problems with them:
>>> 1.) You've copied all the component tests out of the component test
>>> module into the service module.  This means that identical tests will
>>> have to be maintained in two places.  Because people aren't very good
>>> at keeping code in sync, you can be certain that these tests will
>>> diverge from each other a little more each time someone makes a
>>> change, and contributors (myself included) won't know which tests are
>>> authoritative.
>>> 2.) You've removed the Apache license header from at least one file.
>>> I added the Apache license header to the build files very recently, as
>>> one step towards making the code releasable.  We can't release the
>>> code without the ASF license headers.
>> Ouch ! My bad. I would have left this commit as a PR for review first.
>>> 3.) I'm assuming the documentation is generated?  Do we really want to
>>> check in generated documentation?  And where is the process documented
>>> for generating the documentation?
>> By default, the Spring REST Docs tool outputs documentation in the
>> service's build folder ( which is ignored by git ), so I placed them in
>> service/src/doc so they can be viewed.
>>> 4.) You added a compile dependency from service to test.  The service
>>> should not depend on test.  It should be possible to make changes and
>>> add environment variables and etc to the test repository without even
>>> thinking about whether a change might break production code or turn on
>>> test-configurations which weaken security in production code.
>>> In general dependencies between modules must be 1-way, or else
>>> compilation is difficult or impossible.  Component-tests should depend
>>> on service.  Service should not depend on component-test.  Markus
>>> mentioned this when you asked (2) You might ask, why are component
>>> tests a separate module? There are several reasons:
>> I was concerned about duplicating the component-tests' tests in the
>> service module and asked about how to avoid the duplication for which I had
>> no feedback.(2)
>>> * We may wish to release the service and api modules and not release
>>> the component-test module.  This opens up some possibilities for the
>>> tests that would otherwise be closed off by ASF licensing
>>> requirements.
>>> * Keeping the code seperate helps contributors keep the concerns of
>>> testing and the concern of functionality separate in their thought
>>> processes.  Putting testing code in services in other projects has
>>> resulted in poor code or even major security vulnerabilities.  It's
>>> important here to think about solutions and their trade-offs carefully
>>> rather than just slapping things together until they "work" and
>>> calling it a day.
>>> * One of the things I hope to accomplish in the future is versioned
>>> component tests to protect against backwards incompatible changes
>>> which can't be detected via signature checking.  We can't implement
>>> that plan if the tests become entangled with the source code.
>>> As a result I think we need to completely rethink the approach to
>>> documentation that you've taken here.  Some alternatives we should
>>> consider:
>>> * Move mockMVC into component test and out of service.  Why did you
>>> put the documentation and component test code in service in the first
>>> place?
>>> * Create a separate documentation module which imports code from
>>> component-test and service.
>>> * Using Spring Fox instead of MockMVC.  When I originally evaluated
>>> REST documentation approaches, it was exactly this mixing of other
>>> concerns into testing that caused me to reject MockMVC in the first
>>> place.  Tutorials make good tests but there are many, many good tests
>>> which make terrible documentation.
>> I'm okay with the approach to documentation being rethought.
>>> The use of MockMVC came up a few months ago when a potential GSoC
>>> intern suggested it.  I had thought, based on your response, that you
>>> understood why it doesn't work well with our architecture. (1) Which
>>> is why I didn't respond to that thread myself.  What changed?
>>> In general, when making architectural changes which change the pattern
>>> of all of the services it's not a good idea to just dump them into all
>>> the services and then ask for feedback.  You create a lot of
>>> unnecessary work for yourself and for your code reviewer.  A better
>>> approach is to make the change in the fineract-cn-template project
>>> first and then ask for feedback.  Better still would be to engage a
>>> discussion on the mailing list before doing a major architectural
>>> change like this.  If I had known you were working on this, I could
>>> have shared my thoughts before you started.
>> I'm sorry I didn't open a discussion here about documenting the APIs.
>> I'm hoping we'll continue that discussion on this thread and fix the
>> mistakes I made.
>>> Best Regards,
>>> Myrle
>>> 1.) https://lists.apache.org/thread.html/cc23a47229c262afbb6e474
>>> 9b7eec3117d084f84144877d59098713d@%3Cdev.fineract.apache.org%3E
>>> 2.) https://lists.apache.org/thread.html/d0fbdb5b21b916cc8dd3c2a
>>> 5061d3e90aa554bb74b63f8deabe69fe6@%3Cdev.fineract.apache.org%3E
>> At Your Service,
>> Isaac Kamga.
>>> On Mon, Apr 23, 2018 at 8:42 PM, Isaac Kamga <isaac.kamga@xxxxxxxxx>
>>> wrote:
>>> > Hello everyone,
>>> >
>>> > Trust that you're doing great and congratulations to our recently
>>> selected
>>> > GSoC 2018 students.
>>> >
>>> > As you may have observed from Gitbox notifications, I have used the
>>> MockMvc
>>> > flavour of Spring Rest Docs <https://projects.spring.io/sp
>>> ring-restdocs/> to
>>> > document the APIs for most Fineract CN domain services viz identity,
>>> > office, customer, group, accounting, deposit, payroll and teller,
>>> created
>>> > and merged pull requests. There were failing tests in portfolio ( as I
>>> > earlier highlighted last week on the list ) and so snippets weren't
>>> > generated to be used for documentation.
>>> >
>>> > After updating the affected repositories, you can view the
>>> documentation
>>> > for each of the services by opening the
>>> > service/src/doc/html5/html5/api-docs.html file.
>>> >
>>> > I happily await your feedback and enhancements.
>>> >
>>> > At Your Service,
>>> > Isaac Kamga.