Check all repos in Homu's cfg.toml are valid

[rwaweber]

Enumerates the inconsistencies between repositories configured in homu
vs those configured in the servo github organization.

---

This change is 

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @aneeshusa (or someone else) soon.

[jdm]

@aneeshusa or @edunham Could you take a look at this?

[edunham]

The logic looks correct to me, and the whole thing superficially looks like it should work. However, both the master tests fail on Travis.

Could you please add a test in the repo_pager to bail out with a descriptive error message if the auth credentials passed into it didn't work?

(the following is my notes from debugging it -- I might want them later, but you're free to ignore them)

So as you mentioned in your comment on #581, we're sticking a string into the gh-access-token here where we should actually be reading an env var named TEST_HOMU_GH_ACCESS_TOKEN after having set the TEST_HOMU_GH_ACCESS_TOKEN to a working token at https://travis-ci.org/servo/saltfs/settings . So then the string which is not in fact a token at all gets blindly pulled in to the config without being validated as a token here, and is then (i think) successfully read out as if it were the token on line 38 of the new test.

So then the actual error we get is string indices must be integers. I think that means that the req_list is getting filled with strings instead of dicts that you can sanely ask for a html_url out of, and that's happening up in the repo_pager.

[edunham]

If the endpoint returns anything about failed auth, please bail out right here with a descriptive error message.

[rwaweber]

After d83bedf I feel that the need to catch this exception is significantly diminished, since the authorization check is now handled in the repo_pager function.

[aneeshusa]

Sorry about the review delay here, and thanks for the PR! The current approach of enumerating all repos under the servo org has a few problems: it doesn't handle the case of repos that aren't under the servo org, it does more work than necessary, and it requires an authentication token. I think an easier approach would be to just make a HEAD request for each repo to see if it exists, e.g.requests.head('https://github.com/servo/servo').status_code == 200. We only have ~50 repos configured, so this shouldn't be too slow, and is simpler to deploy and understand.

Also, please rebase on top of the latest master to get rid of the merge commit.

[aneeshusa]

The test harness has a blanket catch-all for exceptions raised - these try/except blocks are fine while debugging, but I think they add noise once we get it working.

[rwaweber]

@aneeshusa Thanks for the review!

just make a HEAD request for each repo to see if it exists, e.g.requests.head('https://github.com/servo/servo').status_code == 200

Agreed! I think that would be a great deal simpler. Would also allow me to remove the hackery around the repo_pager function necessary to get through the github api, I must've misunderstood the original request in the issue statement.

Just for the sake of clarity, we only want to make sure that all of the repositories in the homu configuration are configured on github, correct?

Not sure about the rate limiting risk here, as there would now be a request for every repo configured. But, github /probably/ wouldn't rate limit on there http frontend and with ~50 requests. This is something to figure out with testing.

Would it also be preferred to consider using urllib instead of introducing an extra dependency in the form of requests? I'm perfectly happy to rewrite it that way.

[aneeshusa]

Yes, the intent is to ensure all the repositories in the Homu config exist on GitHub.

I didn't think about rate limiting earlier, I thought the token was to get read access to the list of repos under the organization. I do think we'll need a token to work around rate limiting since IIRC there's a limit of 60 requests an hour to the API. However, we can use a separate token for the test suite and just read it from the environment instead of gathering it from the Homu config (see my comment on the PR for the token). Of course, if it works without a token, that's even better!

It would be nice to use urllib and avoid the dependency (especially since it's more ergonomic in Python 3), but I'm also ok with using requests.

[rwaweber]

Lemme know if there's anything you'd want me to change with the above!

From what I can tell(by checking the https://api.github.com/rate_limit endpoint), these requests are not affected by the 60 unauthenticated requests an hour limit imposed by the github api. Downside is, that it makes the homu.sls tests run a good 10-15 seconds slower, which is sort of annoying. Though, getting around that limitation may end up being more trouble than it's worth.

[jdm]

@aneeshusa review ping!

[aneeshusa]

Split up the imports into three groups: Python modules (the urllib imports), external modules (toml) and internal modules (tests.util). Separate each group of imports by a blank line, and alphabetize in each group.

[aneeshusa]

nit: Remove comment

[aneeshusa]

nit: Remove comment

[aneeshusa]

Overall right approach! I left a bunch of Python style nits. Also, prefer block indent for indentation, and capitalize the Success/Failure messages and commit message.

[aneeshusa]

Let's make this a repo_exists method, which takes a repo identifier string in the form owner/name, e.g. servo/saltfs, and returns a boolean. Update the doc string as appropriate.

[aneeshusa]

Use HEAD requests to reduce bandwidth usage - you can create a urllib.request.Request object with method='HEAD', and pass that to urlopen.
Use a with statement for urlopen to automatically close the response.

[aneeshusa]

Iterating over repo_cfg.values() and using the 'owner' and 'name' keys for each repo will make this a little cleaner.

[aneeshusa]

I prefer to use string formatting, e.g. '{}/{}'.format(repo['owner'], repo['name'])

[aneeshusa]

Also, this can be a generator expression instead of making a temporary list.

[rwaweber]

++ on the generator recommendation, I've never used those before so that was neat to play around with them!

[aneeshusa]

Inline this inside the Failureconstructor to avoid constructing it if not necessary.
Use '\n' to join the responses (no preceding space), and precede each entry with a - , which makes a nice list.

[aneeshusa]

Rename this to missing_repos.

[aneeshusa]

Inline this the urllib.request.Request constructor in repo_exists.

[rwaweber]

Hey @aneeshusa, just lemme know if you want me to look at anything else. If not, I'm happy to get down to squashing!

[aneeshusa]

One last nit here! Go ahead and squash when you address this.

[aneeshusa]

one last nit: use block indent for this. This also means the second argument will all fit on one line, i.e.:

    return Failure(
        'Some repos...on GitHub:',
        '\n'.join(' - {}'.format(repo) for repo in missing_repos)
    )

[edunham]

@aneeshusa review ping :)

[aneeshusa]

@rwaweber, all the code looks good! Please update the commit message to have a descriptive title for the change and include some motivation/details as a body. I like https://chris.beams.io/posts/git-commit/ as a guide to good commit messages.

[aneeshusa]

Looks great, thanks for the PR and your patience, @rwaweber!

@bors-servo r+

[bors-servo]

📌 Commit 0acc281 has been approved by aneeshusa

[bors-servo]

⌛ Testing commit 0acc281 with merge d1a5302...

[bors-servo]

☀️ Test successful - status-travis
Approved by: aneeshusa
Pushing d1a5302 to master...

[rwaweber]

Thanks for taking the time to teach, @aneeshusa. Looking forward to the next one!