RAT exclusions for test resources

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RAT exclusions for test resources

Joe Witt
Hello

I've resolved all of the items raised in feedback for the 0.0.2
release excluding one item which will be discussed in a moment.  All
of the feedback and their disposition is captured in this JIRA

https://issues.apache.org/jira/browse/NIFI-410

The only outstanding item is this feedback:

"One thing I'd fix for the next release is the exclusion of test
resources from the RAT check. Wouldn't it be better to do that by file
extension (e.g., */.json, */.avro) to avoid not checking files that
could have license headers?"

I would like to leave it as we have it now.  People should take care
to apply the apache license header to everything including test data
whenever possible.  But we must also be considerate of the fact that
test data is just that - test data.  It is important that data under
test mirrors exactly the data it is to be run against and certainly
data we'd be processing in the wild will not often have license
headers.  Given this I don't see how we can keep the RAT exclusion
list from turning into a mess.  I prefer to trust that the developers
are doing the right thing on test resources and keep the build simple.
For non test resources though we retain a very specific and strict
check.

I'll close the ticket tracking the items raised for now but should
folks have a strong view here then certainly we can address it.

Thanks
Joe
Reply | Threaded
Open this post in threaded view
|

Re: RAT exclusions for test resources

Billie Rinaldi-2
I think it is okay to leave the rat exclusion as is.  However, it is
important that you verify what has been committed as test resources, along
with anything else excluded from the rat check, before release.  I'm sure
that no one will deliberately check in something they know shouldn't be
there, but people have a tendency to commit things to work around policies
they don't fully understand -- and understanding ASF policies is not a
trivial undertaking.  This has happened on every project I've worked on.

On Sat, Mar 14, 2015 at 8:52 PM, Joe Witt <[hidden email]> wrote:

> Hello
>
> I've resolved all of the items raised in feedback for the 0.0.2
> release excluding one item which will be discussed in a moment.  All
> of the feedback and their disposition is captured in this JIRA
>
> https://issues.apache.org/jira/browse/NIFI-410
>
> The only outstanding item is this feedback:
>
> "One thing I'd fix for the next release is the exclusion of test
> resources from the RAT check. Wouldn't it be better to do that by file
> extension (e.g., */.json, */.avro) to avoid not checking files that
> could have license headers?"
>
> I would like to leave it as we have it now.  People should take care
> to apply the apache license header to everything including test data
> whenever possible.  But we must also be considerate of the fact that
> test data is just that - test data.  It is important that data under
> test mirrors exactly the data it is to be run against and certainly
> data we'd be processing in the wild will not often have license
> headers.  Given this I don't see how we can keep the RAT exclusion
> list from turning into a mess.  I prefer to trust that the developers
> are doing the right thing on test resources and keep the build simple.
> For non test resources though we retain a very specific and strict
> check.
>
> I'll close the ticket tracking the items raised for now but should
> folks have a strong view here then certainly we can address it.
>
> Thanks
> Joe
>
Reply | Threaded
Open this post in threaded view
|

Re: RAT exclusions for test resources

Joe Witt
Billie,

Ok understood.  I'll update the release guide to indicate this guidance.

Thanks!
Joe

On Mon, Mar 16, 2015 at 11:10 AM, Billie Rinaldi <[hidden email]> wrote:

> I think it is okay to leave the rat exclusion as is.  However, it is
> important that you verify what has been committed as test resources, along
> with anything else excluded from the rat check, before release.  I'm sure
> that no one will deliberately check in something they know shouldn't be
> there, but people have a tendency to commit things to work around policies
> they don't fully understand -- and understanding ASF policies is not a
> trivial undertaking.  This has happened on every project I've worked on.
>
> On Sat, Mar 14, 2015 at 8:52 PM, Joe Witt <[hidden email]> wrote:
>
>> Hello
>>
>> I've resolved all of the items raised in feedback for the 0.0.2
>> release excluding one item which will be discussed in a moment.  All
>> of the feedback and their disposition is captured in this JIRA
>>
>> https://issues.apache.org/jira/browse/NIFI-410
>>
>> The only outstanding item is this feedback:
>>
>> "One thing I'd fix for the next release is the exclusion of test
>> resources from the RAT check. Wouldn't it be better to do that by file
>> extension (e.g., */.json, */.avro) to avoid not checking files that
>> could have license headers?"
>>
>> I would like to leave it as we have it now.  People should take care
>> to apply the apache license header to everything including test data
>> whenever possible.  But we must also be considerate of the fact that
>> test data is just that - test data.  It is important that data under
>> test mirrors exactly the data it is to be run against and certainly
>> data we'd be processing in the wild will not often have license
>> headers.  Given this I don't see how we can keep the RAT exclusion
>> list from turning into a mess.  I prefer to trust that the developers
>> are doing the right thing on test resources and keep the build simple.
>> For non test resources though we retain a very specific and strict
>> check.
>>
>> I'll close the ticket tracking the items raised for now but should
>> folks have a strong view here then certainly we can address it.
>>
>> Thanks
>> Joe
>>
Reply | Threaded
Open this post in threaded view
|

Re: RAT exclusions for test resources

Ryan Blue
On 03/16/2015 08:13 AM, Joe Witt wrote:

> Billie,
>
> Ok understood.  I'll update the release guide to indicate this guidance.
>
> Thanks!
> Joe
>
> On Mon, Mar 16, 2015 at 11:10 AM, Billie Rinaldi <[hidden email]> wrote:
>> I think it is okay to leave the rat exclusion as is.  However, it is
>> important that you verify what has been committed as test resources, along
>> with anything else excluded from the rat check, before release.  I'm sure
>> that no one will deliberately check in something they know shouldn't be
>> there, but people have a tendency to commit things to work around policies
>> they don't fully understand -- and understanding ASF policies is not a
>> trivial undertaking.  This has happened on every project I've worked on.

I think it would be easier to maintain by adding the individual
exclusions. I don't see a big drawback to having a big longer RAT
configuration, and I'd prefer that to a release task that says to check
all of the src/test/resources files added to do the license check by
hand. A commit with an addition to the RAT rules only needs to be added
once, and a RAT check for other files ensures the source tree is always
up to date.

It's up to you guys in the end, but I don't understand how the general
exclusion makes things easier.

rb


--
Ryan Blue
Software Engineer
Cloudera, Inc.
Reply | Threaded
Open this post in threaded view
|

Re: RAT exclusions for test resources

Joe Witt
Ryan,

To be honest I think you're probably right.  I was being lazy.  But in
reality my laziness would cause an addition step in an already many
step release process.  So that isn't cool. And further my lazy
approach could cause us to miss something.  Your approach requires
more up front cost in the cases where test data does need to bypass
the header but then we're done.  So, in short, i think you're right.

Unless there is any objection i'll go through the current exclusions
and give this a shot.

Thanks
Joe

On Mon, Mar 16, 2015 at 4:57 PM, Ryan Blue <[hidden email]> wrote:

> On 03/16/2015 08:13 AM, Joe Witt wrote:
>>
>> Billie,
>>
>> Ok understood.  I'll update the release guide to indicate this guidance.
>>
>> Thanks!
>> Joe
>>
>> On Mon, Mar 16, 2015 at 11:10 AM, Billie Rinaldi <[hidden email]>
>> wrote:
>>>
>>> I think it is okay to leave the rat exclusion as is.  However, it is
>>> important that you verify what has been committed as test resources,
>>> along
>>> with anything else excluded from the rat check, before release.  I'm sure
>>> that no one will deliberately check in something they know shouldn't be
>>> there, but people have a tendency to commit things to work around
>>> policies
>>> they don't fully understand -- and understanding ASF policies is not a
>>> trivial undertaking.  This has happened on every project I've worked on.
>
>
> I think it would be easier to maintain by adding the individual exclusions.
> I don't see a big drawback to having a big longer RAT configuration, and I'd
> prefer that to a release task that says to check all of the
> src/test/resources files added to do the license check by hand. A commit
> with an addition to the RAT rules only needs to be added once, and a RAT
> check for other files ensures the source tree is always up to date.
>
> It's up to you guys in the end, but I don't understand how the general
> exclusion makes things easier.
>
> rb
>
>
> --
> Ryan Blue
> Software Engineer
> Cloudera, Inc.
Reply | Threaded
Open this post in threaded view
|

Re: RAT exclusions for test resources

Joe Witt
ok i think this is now case closed.  Actions taken

1) Pushed all module specific RAT exclusions down to the specific
relevant modules
2) Removed the broad RAT exclusion for test resources
3) Added specific RAT exclusions per test file for which it was
appropriate based on the test resource in question.
4) Validated full clean build is good to go and that the full RAT
verification works
5) Fixed 3 module directory names which did not match their artifact
names (nifi-cluster -->  nifi-framework-cluster...)

We should now be taking full advantage of RAT and only excluding
specifically approved items.

Ryan: Thanks for being persistent.  You were right.

Thanks
Joe


On Mon, Mar 16, 2015 at 5:03 PM, Joe Witt <[hidden email]> wrote:

> Ryan,
>
> To be honest I think you're probably right.  I was being lazy.  But in
> reality my laziness would cause an addition step in an already many
> step release process.  So that isn't cool. And further my lazy
> approach could cause us to miss something.  Your approach requires
> more up front cost in the cases where test data does need to bypass
> the header but then we're done.  So, in short, i think you're right.
>
> Unless there is any objection i'll go through the current exclusions
> and give this a shot.
>
> Thanks
> Joe
>
> On Mon, Mar 16, 2015 at 4:57 PM, Ryan Blue <[hidden email]> wrote:
>> On 03/16/2015 08:13 AM, Joe Witt wrote:
>>>
>>> Billie,
>>>
>>> Ok understood.  I'll update the release guide to indicate this guidance.
>>>
>>> Thanks!
>>> Joe
>>>
>>> On Mon, Mar 16, 2015 at 11:10 AM, Billie Rinaldi <[hidden email]>
>>> wrote:
>>>>
>>>> I think it is okay to leave the rat exclusion as is.  However, it is
>>>> important that you verify what has been committed as test resources,
>>>> along
>>>> with anything else excluded from the rat check, before release.  I'm sure
>>>> that no one will deliberately check in something they know shouldn't be
>>>> there, but people have a tendency to commit things to work around
>>>> policies
>>>> they don't fully understand -- and understanding ASF policies is not a
>>>> trivial undertaking.  This has happened on every project I've worked on.
>>
>>
>> I think it would be easier to maintain by adding the individual exclusions.
>> I don't see a big drawback to having a big longer RAT configuration, and I'd
>> prefer that to a release task that says to check all of the
>> src/test/resources files added to do the license check by hand. A commit
>> with an addition to the RAT rules only needs to be added once, and a RAT
>> check for other files ensures the source tree is always up to date.
>>
>> It's up to you guys in the end, but I don't understand how the general
>> exclusion makes things easier.
>>
>> rb
>>
>>
>> --
>> Ryan Blue
>> Software Engineer
>> Cloudera, Inc.
Reply | Threaded
Open this post in threaded view
|

Re: RAT exclusions for test resources

Ryan Blue
On 03/16/2015 07:53 PM, Joe Witt wrote:

> ok i think this is now case closed.  Actions taken
>
> 1) Pushed all module specific RAT exclusions down to the specific
> relevant modules
> 2) Removed the broad RAT exclusion for test resources
> 3) Added specific RAT exclusions per test file for which it was
> appropriate based on the test resource in question.
> 4) Validated full clean build is good to go and that the full RAT
> verification works
> 5) Fixed 3 module directory names which did not match their artifact
> names (nifi-cluster -->  nifi-framework-cluster...)
>
> We should now be taking full advantage of RAT and only excluding
> specifically approved items.
>
> Ryan: Thanks for being persistent.  You were right.
>
> Thanks
> Joe

Thanks, Joe!

On second thought, I'd rather more work for the release manager than for
me. Is it too late?

:)

rb


--
Ryan Blue
Software Engineer
Cloudera, Inc.