addon-check icon indicating copy to clipboard operation
addon-check copied to clipboard

Check if all PR files are in the same folder

Open enen92 opened this issue 6 years ago • 17 comments

According to our submissions rules every pull request must refer to only one addon. PRs usually have "[addon.id] version" as the PR title (although in some cases we have to rename them manually or ask the author to do so). Sometimes there is a mistake of including more than one addon in a pull request or of producing incorrect pull requests which include older commits (for example the result of an incorrect rebase).

The CI could check if all the files included in the pull request are constrained to a single folder (with the same name as the addon id); hence detecting if the pull request is valid.

All my issues are just possible features/ideas that come into my mind for the GSOC work on the tool. I can later implement some of them if not addressed during GSOC so please do not interpret them as I am demanding work :).

Regards

enen92 avatar Mar 31 '18 14:03 enen92

It might make sense to only enable this via arg parameters. So that I still can do kodi-addon-checker script.test script.test2 locally, but error on kodi-addon-checker script.test script.test2 --fail-on-multiple-addons

razzeee avatar Mar 31 '18 20:03 razzeee

@enen92 For this we can check if upper directory for all the file is same or not right ? Maybe do something like os.chdir("..") for all the files/folders in depth 1 and then see if that directory is same or not.

mzfr avatar Apr 12 '18 13:04 mzfr

@mzfr Isn't it easier to:

  1. get the list of changed files from the github api associated with the pull request (not familiar with it but I guess you can get them given all the commit hashes) eg:
/home/ci/addons/plugin.video.test/addon.xml
/home/ci/addons/plugin.video.test/resources/lib/plugin.py
....
  1. construct the base path in which the addons are stored in the repository:

addon_dir = Path('/home', 'ci', 'addons')

  1. compute the relative path for each file changed in the PR. eg for one of the paths:
>> relative_path = Path('/home/ci/addons/plugin.video.test/resources/lib/addon.py').relative_to(addon_dir)
>> print(relative_path)
<< PosixPath('plugin.video.test/resources/lib/addon.py')
  1. Grab the basedir:
>> relative_path.parts[0]
<< 'plugin.video.test'
  1. Check if the result for all files is the same. If not the Pull request is incorrect.

enen92 avatar Apr 12 '18 14:04 enen92

@enen92 sorry thats what I meant by "upper directory". But you explained it in detail. Thank you

mzfr avatar Apr 12 '18 14:04 mzfr

I'm not sure about thise, especially if we need to call the github api. It means we add another hard dependency and very closely couple this checker with our repo rules on prs. Not with good rules for coding.

So if we would run this locally with vscode some day it will just tilt untill we do some weird setup. Maybe it's a different concern and would be better of in it's own tool?

razzeee avatar Apr 12 '18 20:04 razzeee

add the following to .travis.yml - 'if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then if [ "$(git diff --diff-filter=d --name-only HEAD~ | grep / | cut -d / -f1 | sort | uniq | wc -l)" -gt "1" ]; then exit 1; fi fi'

Rechi avatar Apr 13 '18 07:04 Rechi

@Razzeee @enen92 This is done right ?

mzfr avatar Apr 28 '18 17:04 mzfr

No, I haven't updated our travis scripts yet

razzeee avatar Apr 29 '18 09:04 razzeee

If you can tell me for what all branches travis script are to need to be updated then I can do it :slightly_smiling_face:

mzfr avatar May 05 '18 15:05 mzfr

Well we need to add the stuff posted by rechi above and maybe also add the needed travis-buddy command, that I already added as a test on the plugins repo for the krypton branch.

razzeee avatar May 06 '18 22:05 razzeee

@Razzeee any updates on this one ?

mzfr avatar Aug 24 '18 17:08 mzfr

@Rechi is that code snippet still how it should look?

razzeee avatar Oct 01 '18 15:10 razzeee

Yes, but it would mark some of our own PRs in repo-resources as failed (e.g. mass updates to game.controler.*).

Rechi avatar Oct 01 '18 16:10 Rechi

So we might want to not do this I guess.

razzeee avatar Oct 03 '18 09:10 razzeee

I think we still need this, mass updates to addons are not common and are usually handled only by team members. In most situations 1 PR = 1 addon update. Would it be possible to add a label to a pull request and make travis ignore the check? Something like the "No-Jenkins" label in Jenkins?

enen92 avatar Oct 07 '18 09:10 enen92

Here is the script which just checks every commit of a PR separately.

if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then
        git rev-list --no-merges --reverse HEAD~..HEAD | while read rev; do
                if [ "$(git diff --diff-filter=d --name-only $rev~..$rev | grep / | cut -d / -f1 | sort | uniq | wc -l)" -gt "1" ]; then
                        echo 'The following commit changes more then one add-on'
                        git --no-pager show --stat $rev
                        exit 1
                fi
        done
fi

Rechi avatar Nov 05 '18 14:11 Rechi

@Rechi This script is all we need to add in the .travis.yml file right? or is there any other changes required to fix the issue?

mzfr avatar Dec 10 '18 14:12 mzfr