> On Jan. 27, 2015, 3: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.
> 
> kturner wrote:
>     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.

We have to consider the target audience. For build instructions for the target 
audience of "consumers of the source tarball" (the official release), I think 
the BUILD.md file makes sense. However, these instructions I've highlighted do 
not target that audience (as stated explicitly in the paragraph under the 
`Iterative Experimentation` header). I think these instructions would confuse 
the primary target audience and should not be in the BUILD.md file. It is 
useful, though, and we need to put it somewhere... I just think that somewhere 
should be in a developer section on the website, rather in the BUILD.md file 
targeted towards non-developers.


> On Jan. 27, 2015, 3: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.
> 
> kturner wrote:
>     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.

I think the INSTALL.md primary target audience is first-time installers. 
Upgrade-related caveats and items to take notice of seem appropriate for the 
release notes. It's probably fine to include here as well. I just have more 
often seen projects provide upgrade instructions/notes on the website.

> I do not think docs on the website have a better chance of being maintained 
> vs docs in src.

In general, I agree. But, that's not the case for release notes... which get 
attention on every release.


> On Jan. 27, 2015, 3: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.
> 
> kturner wrote:
>     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).

I just think it's unusual to describe how to execute each individual phase in 
the maven build lifecycle. That's what I mean by "maven tutorial". The build 
instructions describe `mvn package`. Here, it should just describe `mvn 
verify`. After all, the integration tests get executed during the 
integration-test phase of the build lifecycle, but they don't get checked for 
errors until the verify phase anyway. So, integration testing is not actually 
completed unless you execute `mvn verify`. So, some of this is technically 
correct, but misleading, and doesn't do justice the actual behavior of these 
goals.

> Do you want to just remove the following?

That would help, but I also think it would be better to simply describe `mvn 
verify` instead of the other goals.


- Christopher


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


On Jan. 28, 2015, 12:21 p.m., kturner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30236/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2015, 12:21 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
> 
>

Reply via email to