Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if all PR files are in the same folder #60

Open
enen92 opened this issue Mar 31, 2018 · 17 comments
Open

Check if all PR files are in the same folder #60

enen92 opened this issue Mar 31, 2018 · 17 comments

Comments

@enen92
Copy link
Member

enen92 commented Mar 31, 2018

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

@razzeee
Copy link
Member

razzeee commented Mar 31, 2018

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

@mzfr
Copy link
Contributor

mzfr commented Apr 12, 2018

@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.

@enen92
Copy link
Member Author

enen92 commented Apr 12, 2018

@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.

@mzfr
Copy link
Contributor

mzfr commented Apr 12, 2018

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

@razzeee
Copy link
Member

razzeee commented Apr 12, 2018

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?

@Rechi
Copy link
Member

Rechi commented Apr 13, 2018

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'

@mzfr
Copy link
Contributor

mzfr commented Apr 28, 2018

@razzeee @enen92 This is done right ?

@razzeee
Copy link
Member

razzeee commented Apr 29, 2018

No, I haven't updated our travis scripts yet

@mzfr
Copy link
Contributor

mzfr commented May 5, 2018

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

@razzeee
Copy link
Member

razzeee commented May 6, 2018

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.

@mzfr
Copy link
Contributor

mzfr commented Aug 24, 2018

@razzeee any updates on this one ?

@razzeee
Copy link
Member

razzeee commented Oct 1, 2018

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

@Rechi
Copy link
Member

Rechi commented Oct 1, 2018

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

@MartijnKaijser MartijnKaijser added this to To do in Roadmap via automation Oct 1, 2018
@MartijnKaijser MartijnKaijser moved this from To do to Done in Roadmap Oct 1, 2018
@MartijnKaijser MartijnKaijser moved this from Done to To do in Roadmap Oct 1, 2018
@razzeee
Copy link
Member

razzeee commented Oct 3, 2018

So we might want to not do this I guess.

@enen92
Copy link
Member Author

enen92 commented Oct 7, 2018

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?

@Rechi
Copy link
Member

Rechi commented Nov 5, 2018

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

@mzfr
Copy link
Contributor

mzfr commented Dec 10, 2018

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Roadmap
  
To do
Development

No branches or pull requests

5 participants