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.

Sign up for GitHub

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

add new boolean GH_COMMENT env var for whether to post GitHub comments #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add new boolean GH_COMMENT env var for whether to post GitHub comments #7

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68eaadf
add new boolean GH_COMMENT env var for whether to post GitHub comments
snarfed May 18, 2016
b7acb6c
review feedback in #7: refactor GH_COMMENT constant check
snarfed May 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .env.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ GH_SECRET=moresecrets
GH_ORG=29205
GH_ORG_NAME=localytics
GH_REPOS=repo1,repo2
GH_COMMENT=true
LANGS=rb,js;rb
MODE=proxy
MONGO_URI=mongodb://heroku_appxyz:[email protected]:31978/heroku_appxyz
Expand Down
4 changes: 4 additions & 0 deletions app.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@
"GH_REPOS": {
"description": "Comma-separated Github repo names to track coverage for"
},
"GH_COMMENT": {
"description": "Whether to comment on GitHub PRs",
"value": "true"
},
"LANGS": {
"description": "Comma-separated language file extensions (such as rb,js) to collect coverage for (semicolon-separated for multiple repos, one per repo)"
},
Expand Down
12 changes: 7 additions & 5 deletions app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@
LANGS = dict(zip(constants.get('GH_REPOS').split(','), constants.get('LANGS').split(';')))
CURRENT = dict(zip(constants.get('GH_REPOS').split(','), constants.get('CURRENT').split(';')))

try:
s3 = S3(constants.get('AWS_ACCESS_KEY'), constants.get('AWS_SECRET_KEY'), constants.get('AWS_BUCKET'))
except:
s3 = None
s3 = None
if constants.get('GH_COMMENT').lower() == 'true':
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't actually need any of the changes in this file, because connecting to S3 will fail if the constants are not set. Then later on we can keep the old if not s3. This de-couples S3 usage from commenting, in case we want it for something else later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok! makes sense. the one reason i tied them together was to avoid the red 'Your S3 keys are invalid!' error message, since we intentionally aren't using S3. should i add another constant for that? open to other ideas!

try:
s3 = S3(constants.get('AWS_ACCESS_KEY'), constants.get('AWS_SECRET_KEY'), constants.get('AWS_BUCKET'))
except:
pass

collections = zip(constants.get('GH_REPOS').split(','), constants.get('STORAGE_COLLECTIONS').split(','))
storages = {}
Expand Down Expand Up @@ -50,7 +52,7 @@ def preprocess_request():
return redirect(url_for('login_view'))
if session.get('next'):
return redirect(session.pop('next'))
if not s3:
if constants.get('GH_COMMENT').lower() == 'true' and not s3:
flash('Your S3 keys are invalid!', 'danger')
return 'Your S3 keys are invalid!'

Expand Down
11 changes: 6 additions & 5 deletions helpers/githubbot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,12 @@ def comment(self, pull_request_id, message, url, args, storage, coverage_diffs,
self.post_comment(body, pr)

def post_comment(self, body, pr):
past_comment = self.past_comment(pr)
if past_comment:
past_comment.edit(body)
else:
pr.create_issue_comment(body)
if self.constants.get('GH_COMMENT').lower() == 'true':
Copy link
Owner

@yasyf yasyf May 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking this each time, maybe do self.post_comments = constants.get('GH_COMMENT').lower() == 'true' in __init__ and just do the following here.

def post_comment(self, body, pr):
  if not self.post_comments:
    return

  past_comment = self.past_comment(pr)
  if past_comment:
    past_comment.edit(body)
  else:
    pr.create_issue_comment(body

past_comment = self.past_comment(pr)
if past_comment:
past_comment.edit(body)
else:
pr.create_issue_comment(body)

def get_pr_by_branch(self, branch_name):
cached = self.cache['prs'].get(branch_name)
Expand Down