The future of `validate all`

daveverwer commented 4 years ago

I know we removed the documentation for all with this release but I had forgotten the subtleties of how it worked when I came to test it. I know we chatted on Slack about it, and I remember now, but I'd like to propose that we either:

Remove all processing from the tool completely. It currently reports 50+ false-positive errors due to non-master branch names which really makes the subcommand pretty useless. With the shift away from master branch names, especially with GitHub changing their default branch name, this is going to become a bigger problem over time.
Make all work with a GITHUB_TOKEN environment variable. If set, pass that to a GitHub API call and check default branch names even when checking all. The GitHub API allows 5,000 API calls/hour, so this should last us for quite some time.

I honestly don't have a preference for which of these we choose. I think all is of limited use and I don't think the tool would suffer significantly if it were removed. But, if you're keen to keep it, I think it needs to be aware of default branch names.

Note: If we do go down the route of implementing the GitHub token, we should use GITHUB_TOKEN as the specific name so it fits with the same ENV variable we use in the SPI-Server project.

leogdion commented 4 years ago

It does actually work with:

GITHUB_API_USERNAME=leogdion@brightdigit GITHUB_API_TOKEN=***** swift run swiftpmls all SwiftPMLibrary/packages.json

So the TOKEN is implemented just a different name. Also I believe you need a user name as well for it to work. Correct me if I am wrong.

daveverwer commented 4 years ago

No username needed, as far as I know. Just the personal token.

So if the token is supplied, it does do the GitHub checks?

leogdion commented 4 years ago

Here's where I see the username required: https://developer.github.com/v3/auth/

all doesn't check for branches but I can set that up easily. However I'd like a fallback in case folks go over the api limit. diff uses the environment variables if supplied.

daveverwer commented 4 years ago

/cc @finestructure on the username issue?

I don't think the fallback is a particularly good idea. If it's going to result in false positives, which it will, it would be better for it to be explicit about failing. It won't fail unless you run an all validation more than twice per hour.

finestructure commented 4 years ago

We're using the "Authenticating for SAML SSO", which doesn't need anything other than the token: https://developer.github.com/v3/auth/#authenticating-for-saml-sso

leogdion / PackageListValidator

The future of `validate all` #15