Amphtml: Consider integrating ampproject/amphtml with lgtm analysis

[rsimha]

From #10090

ltgm.com provides a mechanism for automatic code reviews for github projects, and has detected a bunch of errors in ampproject/amphtml at https://lgtm.com/projects/g/ampproject/amphtml/alerts/

There is a way to integrate lgtm with amphtml PRs at https://lgtm.com/docs/lgtm/config/pr-integration

Let's do it if we think the alerts generated for the amphtml repo are useful.

[rsimha]

/cc @cramforce @erwinmombay

[rsimha]

@cramforce, most of our alerts seem to be stemming from ads/ads.extern.js . Are these valid alerts, and if so, is there any reason we're evaluating all those terms without using them?

[cramforce]

No, we should whitelist *.extern.js as these are only superficially valid JS :)

[rsimha]

Per @cramforce, it appears that lgtm needs admin access to ampproject/amphtml for CI integration.

@xiemaisi, is there a way around this where the integration can be done with just commit read/write access?

[xiemaisi]

@cramforce: if you add @externs annotations to those files lgtm will stop complaining.

@rsimha-amp: I believe admin access is currently required, @samlanning should be able to provide more details.

[s0]

Hi @rsimha-amp

Unfortunately not. The way that GitHub integrations work require us to set up a webhook on a repository on GitHub, that will then send an HTTP request to our servers whenever a pull request is opened or modified. To do this, an admin/owner of the repository (someone who has access to the repository settings) needs to be the person to set this up.

[rsimha]

@samlanning, just to be sure that I understood you correctly, can you comment on which of these statements is true?

1. Only the admin / owner of the ampproject/amphtml GH repository can set up integration with lgtm
2. The lgtm plugin needs to be granted admin access to ampproject/amphtml for the integration to work, and cannot make do with just committer read/write access

If 1 is true but 2 is false, @cramforce has admin access and can help set things up. However, if 2 is true, I'm afraid that's a deal breaker.

[s0]

correct, 1 is true, 2 is false.

The only permissions that are requested by lgtm when an admin hits the button that sets up PR integration is:

• Permission to set up webhooks
• Permission to post commit statuses

We don't even need to request read / write access to the repository, since we're not adding commits, and we're only currently integrated with public repositories.

[cramforce]

@samlanning Github recently started allowing you to do what you need without admin access. We cannot grant this by policy. We recently installed https://percy.io/ which is very similar to LGTM and doesn't need admin access

[s0]

@cramforce

To clarify, lgtm does not request admin access to the whole repository, just for the ability to add and remove repository hooks. The permissions we ask for are admin:repo_hook and repo:status.

admin:repo_hook is considered an "admin" permission because it allows for us to delete webhooks too. The reason we ask for this is so that if you choose to uninstall PR integration via our website, we can uninstall the webhook from your repository.

I understand that this may be against your policy, and unfortunately, for now this is the only way in which you can set up lgtm integration.

We are very excited about the new GitHub Apps API, which as you said would solve this problem for us entirely. It's something we'd love to start using, but sadly for the time being it's not on our development schedule.

[cramforce]

Could we just manually set up the webhooks?

On Thu, Jun 22, 2017 at 2:05 PM, Sam Lanning [e-mail]
wrote:

@cramforce https://github.com/cramforce

To clarify, lgtm does not need request admin access to the whole
repository, just for the ability to add and remove repository hooks. The
permissions we ask for are admin:repo_hook and repo:status.

admin:repo_hook is considered an "admin" permission because it allows for
us to delete webhooks too. The reason we ask for this is so that if you
choose to uninstall PR integration via our website, we can uninstall the
webhook from your repository.

I understand that this may be against your policy, and unfortunately,
for now this is the only way in which you can set up lgtm integration.

We are very excited about the new GitHub Apps API, which as you said would
solve this problem for us entirely. It's something we'd love to start
using, but sadly for the time being it's not on our development schedule.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/issues/10092#issuecomment-310502571,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT_q3dX2RdNIzqf-Mp9mElVQ6dnMbks5sGtc1gaJpZM4OChD9
.

[s0]

Unfortunately this isn't quite as trivial as it may sound, and there are a number of problems with this:

• Firstly, we loose the HMAC functionality that we use to authenticate notifications are genuinely from github (we send a Pre-Shared key for use in the HMAC from lgtm.com to GitHub when setting up the webhooks). Sending the PSK over email, etc... does not seem like a good idea.
• We still need to be granted oauth tokens that are authorised to post commit statuses, and manually put this into lgtm's database.

We're having some internal discussions about some short/medium term improvements to this (i.e. until we start using the Apps API). We'll post here if anything changes.

[cramforce]

Thanks. For what its worth, we have no problem granting access to commit statuses at all!

[xiemaisi]

@cramforce, @rsimha-amp: I understand we no longer require admin permissions for setting up PR integration; would you like to give it another go?

[cramforce]

Thanks. Done.

[rsimha]

@xiemaisi, @samlanning, thanks for the help!

[rsimha]

@xiemaisi, @samlanning, I have a follow up question.

How does a member of ampproject/amphtml see lgtm results from past PRs? When I go to https://lgtm.com/projects/g/ampproject/amphtml/ci/, I see a warning about not having admin access even though I'm signed in to lgtm with my github account. I know there's at least one successful run that I'd expect to have shown up on that list.

I'm looking for something similar to the list Travis shows. Does an admin need to explicitly invite members to view past lgtm results?

[s0]

Hi @rsimha-amp

Currently there is no view on lgtm.com that lets you see all PR analyses that have been run, like the list travis shows. The only way to currently see results is via the status links on github.

(we do plan on changing this in the future and adding such a view)

[rsimha]

@cramforce FYI, I'm fixing all the LGTM errors in #12597.

@samlanning I've noticed that over the past few months, errors have snuck in to our code because the lgtm check either takes a really long time (therefore, we can't make it a required check) , or because it remains stuck in the Running analyses for revisions state. or because it shows a green check mark against PRs that introduce errors.

For example, see #12544, which introduced these errors, but remained stuck in the Running analyses... state.

Or see #11830, which introduced these errors, but showed a green check mark even though there were new errors.

I think it would be useful if the lgtm check were to show a red X mark in cases where a PR introduces new errors. WDYT? Edit: Looks like we can enable comments on PRs from LGTM. See my comment below.

[rsimha]

@cramforce I think we can avoid future errors by allowing LGTM to post comments on pull requests when an error is found.

This can be done via the lgtm integration settings for ampproject/amphtml, by unchecking the Deactivate comments checkbox. See https://lgtm.com/docs/lgtm/using-lgtm-analysis-continuous-integration

Edit: Also see https://lgtm.com/projects/g/ampproject/amphtml/ci/

[cramforce]

I activated comments.

[rsimha]

@sjvs, @xiemaisi, @samlanning, I'm hoping one of you can help solve this issue we've run into with LGTM.com / GitHub integration.

As you can see from this comment by @cramforce (owner of ampproject/amphtml), he set up pull request integration and comment integration with his account. However, it's becoming tedious for him to manage all his Github notifications due to how LGTM.com adds comments using the admin's account.

So I went ahead and set up a separate account (github.com/amphtml-lgtm-bot), which is also an admin of ampproject/amphtml, so that I could set up LGTM.com integration via that account. I'm seeing that the account is indeed an admin...

... but when I log in to LGTM.com with that account, I see the "not admin / owner" error message.

I tried everything that was suggested at the help page, with no luck. Any idea what gives? (For now, I've moved LGTM.com integration from @cramforce to my account @rsimha, so I'm being notification spammed. Help!)

[rsimha]

/cc @lgtmhq ^^

[s0]

Hi @rsimha, this is likely due to our caching mechanisms... did you originally log in and view this page from this account before you gave it admin access?

(this particular cache has an abnormally long expiry of 24hrs, we should change this). In any case i've cleared the cache now, can you see if this fixes it? You may need to refresh a couple of times.

[rsimha]

@samlanning That worked. Thanks!

Follow up question: Clearly, the amphtml-lgtm-bot account needed to have admin access to our GitHub repository in order to initially set up LGTM.com integration. However, does it need admin access after the integration is set up? Can I downgrade the account from admin access to write access, and have the GitHub integration continue to work?

[s0]

@rsimha Awesome!

And no I don't think you can downgrade, it needs admin access to be able to set the commit statuses I believe?

[rsimha]

@samlanning Sounds good. I'll leave the account with admin access. Thanks for the help!

For posterity, here's an example of what incoming LGTM.com comments to ampproject/amphtml will look like: https://github.com/ampproject/amphtml/pull/14339#issuecomment-377666805

/cc @cramforce