git.net

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

Re: Apache Fineract CN API Documentation


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