Skip to content

Auto comment on issues found in pull request#34

Merged
razzeee merged 5 commits intoxbmc:masterfrom
mzfr:auto-comment
Feb 28, 2018
Merged

Auto comment on issues found in pull request#34
razzeee merged 5 commits intoxbmc:masterfrom
mzfr:auto-comment

Conversation

@mzfr
Copy link
Contributor

@mzfr mzfr commented Feb 11, 2018

It fixes issue #27

This will help in informing contributors about the problems in their add-ons.

I have tested it locally but haven't tested it on Travis-CI.

elif error_counter["warnings"] > 0:
# If we only found warnings, don't mark the build as broken
colorPrint("We found %s problems and %s warnings, please check the logfile." % (
error_counter["problems"], error_counter["warnings"]), "35")

This comment was marked as spam.

This comment was marked as spam.

git_comments.py Outdated
# These are default Travis variables identifying a pull request
# https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables
repo = os.environ["TRAVIS_REPO_SLUG"]
pr = os.environ["TRAVIS_PULL_REQUEST"]

This comment was marked as spam.

check_addon.py Outdated

def _logProblem(error_counter, problem_string):
colorPrint("PROBLEM: %s" % problem_string, "31")
comments.append("PROBLEM: %s" % problem_string)

This comment was marked as spam.

@razzeee
Copy link
Member

razzeee commented Feb 11, 2018

Thank you for this, looks very promissing. I will try to run it on my github branch via travis as soon as you are able to fix the minors :)

@mzfr
Copy link
Contributor Author

mzfr commented Feb 12, 2018

I've fixed the issues and also tested on travis

check_repo.py Outdated
addon_path = os.path.join(repo_path, addon_folder)
error_counter = check_addon.start(error_counter, addon_path, config)

if check_addon.comments:

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@mzfr
Copy link
Contributor Author

mzfr commented Feb 15, 2018

Made few changes suggested by @MartijnKaijser and @razzeee

@mzfr
Copy link
Contributor Author

mzfr commented Feb 16, 2018

@razzeee I have added option for .test-config.json.

I have tested it on a config file similar to this:

{
    "check_legacy_language_path": false,
    "check_legacy_strings_xml": true,
    "check_license_file_exists": true,
    "comment_on_pull": true
}

@mzfr
Copy link
Contributor Author

mzfr commented Feb 24, 2018

@razzeee Are there any other changes required in this ?

@razzeee
Copy link
Member

razzeee commented Feb 25, 2018

I'm a bit short on review time at the moment. The only thing you could do is sqash your commits if you know how to do that :)

@razzeee
Copy link
Member

razzeee commented Feb 25, 2018

I think the travis ci file also needs changes as you added some dependencies that are not installed on ci otherwise

@mzfr mzfr force-pushed the auto-comment branch 4 times, most recently from be0ae6f to fb30e47 Compare February 26, 2018 14:34
@razzeee
Copy link
Member

razzeee commented Feb 26, 2018

I took the liberty to add requests in the travis.yml examples

@razzeee
Copy link
Member

razzeee commented Feb 26, 2018

It's not perfect yet, check https://github.com/Razzeee/repo-plugins/pull/1 for an example.

I'm not sure if it might be prettier to show the errors in a markdown list? Might get very long, but at least you don't need to scroll horizontally.

check_repo.py Outdated
GithubAPI().remove_label(["Checks failed"])
GithubAPI().set_label(["Checks passed"])
else:
colorPrint("No config file found", "7")

This comment was marked as spam.

This comment was marked as spam.

@razzeee
Copy link
Member

razzeee commented Feb 28, 2018

Tested a bit more and it's failing quiet often due to

{'message': "API rate limit exceeded for 52.45.185.117. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)", 'documentation_url': 'https://developer.github.com/v3/#rate-limiting'}

@mzfr
Copy link
Contributor Author

mzfr commented Feb 28, 2018

@razzeee I have made the changes so now API Rate limit have increased from 60 to 5000(It's the maximum).

@razzeee
Copy link
Member

razzeee commented Feb 28, 2018

Seems awy more relyable 🎉

But formatting is still off https://github.com/Razzeee/repo-plugins/pull/1#issuecomment-369303762

check_addon.py Outdated

def _logProblem(error_counter, problem_string):
colorPrint("PROBLEM: %s" % problem_string, "31")
comments.append("PROBLEM: %s" % problem_string)

This comment was marked as spam.

@mzfr
Copy link
Contributor Author

mzfr commented Feb 28, 2018

@razzeee I have made changes suggested by you

@razzeee
Copy link
Member

razzeee commented Feb 28, 2018

This is perfect, thank you 🌮

And sorry for taking so long to review and having you go back so many times 💃

@razzeee razzeee merged commit acc1986 into xbmc:master Feb 28, 2018
@razzeee
Copy link
Member

razzeee commented Mar 11, 2018

Crap, would have been to easy.
Just tried to set it up and it keeps failing due to the GITHUB_TOKEN enviroment variable not being set anymore.
https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions

@mzfr
Copy link
Contributor Author

mzfr commented Mar 12, 2018

@razzeee Wow, this is bad. There must be some recourse against this, right? Considering so many other tools do this GitHub comments thing.

At worst, we would have to stop using Travis CI (just for commenting; we can continue using it for builds) and maybe dedicate our own server for commenting, which would work via GitHub/Travis events hook API. There are existing tools for this or we could try to come up with our own lean implementation in Python. for. eg. this is how TravisBuddy's comments look, notice that they are on a pull created from a fork of the repo.

But perhaps there is a better way to deal with this?

@MartijnKaijser
Copy link
Member

I think we'll have to add some github hook which we can grant access to be able to comment

@razzeee
Copy link
Member

razzeee commented Mar 12, 2018

Well that travis buddy is running on their own infrastructure which obviously can keep the api key secret. While we try to let travis do that for us :/

@mzfr
Copy link
Contributor Author

mzfr commented Mar 12, 2018

Yeah, but Travis isn't able to fulfill what we want, right? and I looked into this a bit more and it seems like tools like TravisBuddy are our best bet but yes we'll need our own infrastructure for that, if we want to customise the comments that we post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants