accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ke...@deenlo.com
Subject Re: Review Request 30236: Reorganize README
Date Wed, 28 Jan 2015 17:11:59 GMT


> On Jan. 27, 2015, 8:04 p.m., Christopher Tubbs wrote:
> > BUILD.md, lines 35-71
> > <https://reviews.apache.org/r/30236/diff/5/?file=836993#file836993line35>
> >
> >     I'm not sure this section is even necessary here. It seems more appropriate
to developer documentation on the website.

Its useful information, so it needs to go somewhere.  Build instructions for 1.4, 1.5 and
1.6 are very different.  Its nice when dealing with those older versions to be able to look
at their build instructions in the README.  I am not sure if version specific build instructions
would be properly maintained on the website.   IMO keeping these instructions versioned with
source is perferable to putting them on the website.  If we do keep these instructions in
source, I think BUILD.md is an ok place.


> On Jan. 27, 2015, 8:04 p.m., Christopher Tubbs wrote:
> > INSTALL.md, lines 152-223
> > <https://reviews.apache.org/r/30236/diff/5/?file=836994#file836994line152>
> >
> >     I would like to see upgrade hints/tips/suggestions/procedures to be documented
in the release notes on the website.
> >     
> >     Perhaps they are appropriate here also, but I feel like those are going to be
very version-specific, and these README files aren't going to get a lot of attention over
time, and this information is going to get stale and/or lengthy and confusing.

The instructions will need to be updated for 1.7, could create a follow on issue about 1.7
upgrade instructions that includes this.  I suppose one option is to not carry these instructions
forward for now, and refer to them in the old README when the 1.7 upgrade instructions are
written.

> these README files aren't going to get a lot of attention over time

Well I hope this reorganization of the content will encourage maintenance (or no longer discourage
maintenance, as I feel the previous state of things did).  I do not think docs on the website
have a better chance of being maintained vs docs in src.


> On Jan. 27, 2015, 8:04 p.m., Christopher Tubbs wrote:
> > TESTING.md, lines 20-53
> > <https://reviews.apache.org/r/30236/diff/5/?file=836999#file836999line20>
> >
> >     This whole section feels like a maven tutorial. It might be sufficient to mention
that unit tests are executed by the maven-surefire-plugin, and integration tests are executed
by the maven-failsafe-plugin, and a link to the "Introduction to the Maven Build Lifecycle"
site, and each of those plugins for more information.
> >     
> >     Some of the other non-maven content, like the minimal requirements, and the
run-length seem appropriate and relevant, but there's a lot of detail here about how standard
maven plugins are working, and there are better resources for that information than our particular
execution of them.

oh no more comments about TESTING.md.  All I did in this patch was rename it from TESTING
to TESTING.md :)  However I am ok with reviewing and improving it here, its just that all
of the comments about it were completely unexpected.

I don't feel like the following long sentence is equivalent to a maven tutorial.  I can work
the links in, but I see no harm in leaving it.

    Integration tests can be run by invoking `mvn integration-test` at the top of the Apache
Accumulo source tree; however,
    like `mvn package` being recommended for unit tests, `mvn verify` is often the recommended
avenue to run the integration tests.

Do you want to just remove the following?

    Take note that when invoking the `integration-test` lifecycle phase, other functions will
also be enabled which include
    static analysis (findbugs) and software license checks (release analysis tool -- RAT).


> On Jan. 27, 2015, 8:04 p.m., Christopher Tubbs wrote:
> > TESTING.md, lines 30-32
> > <https://reviews.apache.org/r/30236/diff/5/?file=836999#file836999line30>
> >
> >     `mvn test` doesn't even work in our build, IIRC, due to the multi-module non-jar
dependency for the native-maps. It's probably sufficient to simply suggest `mvn package`,
and note that it executes the maven build lifecycle through unit testing phase.

Next patch will have an update related to this.  However I would like you to review them.
 Will leave issue open for now as a reminder.  Can close this after you look at changes. 
 Open another issue against new patch if you see more improvements.


- kturner


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30236/#review69865
-----------------------------------------------------------


On Jan. 27, 2015, 5:22 p.m., kturner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30236/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2015, 5:22 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-1515
>     https://issues.apache.org/jira/browse/ACCUMULO-1515
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> Reorganized information in README and converted to markdown.  
> 
> At this point I like the INSTALL.md document, but do not really like the content of the
README.md ATM.  Putting this up for review to get suggestions.
> 
> See how the markdown looks on GH : https://github.com/keith-turner/accumulo/tree/ACCUMULO-1515
> 
> 
> Diffs
> -----
> 
>   BUILD.md PRE-CREATION 
>   INSTALL.md PRE-CREATION 
>   NOTICE af212c2 
>   README 4ebb078 
>   README.md PRE-CREATION 
>   TESTING cf2afba 
>   TESTING.md PRE-CREATION 
>   assemble/src/main/assemblies/component.xml 3f18da3 
> 
> Diff: https://reviews.apache.org/r/30236/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> kturner
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message