Fix loose forbidden file #339
Conversation
| @@ -219,29 +220,22 @@ public boolean isInteresting(String project, String branch, String topic, List<S | |||
| boolean foundInterestingTopicOrFile = false; | |||
There was a problem hiding this comment.
I don't think these are used any more?
| return true; | ||
| } | ||
| } | ||
| return isInterestingTopic(topic) && !files.isEmpty() && isInterestingFile(files); |
There was a problem hiding this comment.
I'm not sure we want to return false if there are no changed files. Only if there are files we are interested in and changed files is empty.
There was a problem hiding this comment.
The condition you indicate is handled in isInterestingFile().
This test is here because we should return false when all files are forbidden, i.e. there are no more after filtering.
When `disableStrictForbiddenFileVerification` is set and the commit contains only forbidden files, the build should not be triggered. This can happen when setting forbidden files but not allowed files, i.e. marking all files as interesting.
francoisferrand
force-pushed
the
fixLooseForbiddenFile
branch
from
43f85dc
to
6b6af61
Compare
|
@rsandell : I have updated the patch, can you check it again? Thanks, |
| return false; | ||
| } else { | ||
| foundInterestingForbidden = true; | ||
| i.remove(); |
There was a problem hiding this comment.
Potentially this line creates a bug. Because https://github.com/sonyxperiadev/gerrit-events/blob/7f3547c6d55946e25e99a847b5160d69e59994ba/src/main/java/com/sonymobile/tools/gerrit/gerritevents/dto/events/ChangeBasedEvent.java caches the list of files and checks it against null. So, one trigger modifies list of files visible for another trigger.
When
disableStrictForbiddenFileVerificationis set and thecontains only forbidden files, the build should not be triggered. This
can happen when setting forbidden files but not allowed files, i.e.
marking all files as interesting.