feature: Validate incoming policy definition in PR changes #448
Conversation
bmoylan
force-pushed
the
bm/validate-head-changes
branch
from
2f3721a
to
a8b9daa
Compare
|
Unfortunately, the best way to test handler code is with a running instance. We don't have any kind of tests that use mock GitHub events at this time. If you don't have a local dev instance, we can probably test in staging after merging this. |
There was a problem hiding this comment.
One reason I've been a bit hesitant about this feature is that it will cause all evaluations to load all the files in the pull request. If the policy doesn't otherwise have any rules that use file paths, this could be many extra requests for a limited use case. But in practice, I'm not sure what percentage of policies have no file conditions.
I'm trying to think of way to optimize this without adding too much complexity. One thought is to look at the incoming trigger and only validate files for triggers that could have changed content (i.e. TriggerCommit.) Unfortunately, I think this means that if you have an invalid policy, and then someone approves the PR or another evaluation happens, it could overwrite the failure from the bad policy.
Maybe there's an alternate way to check for policy modifications? Always fetching the policy file from the head branch and then comparing the content against the base branch might be faster.
|
|
||
| files, err := prctx.ChangedFiles() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
One of the conditions this can fail in is when a pull request modifies more than 3000 files. My instinct is that validation of policy changes should be best effort and that we should not fail the overall evaluation for conditions like this if the actual policy may not be impacted by the same limit.
What do you think? I can see how it might be misleading if an invalid policy resulted in a passing check due to an error with the GitHub API or something.
| switch f.Status { | ||
| case pull.FileAdded, pull.FileModified: | ||
| switch f.Filename { | ||
| case b.PullOpts.PolicyPath, ".github/" + b.PullOpts.PolicyPath: |
There was a problem hiding this comment.
I think PolicyPath is the only path that we need to check for here
|
|
||
| fetchedConfig := b.ConfigFetcher.ConfigForPRHead(ctx, prctx, client) | ||
| _, err = b.ValidateFetchedConfig(ctx, prctx, client, fetchedConfig, trigger) | ||
| return err |
There was a problem hiding this comment.
I'm not sure if the error returned from ValidateFetchedConfig makes it clear where the error was. We may want to add an additional message to clarify that this is from the policy being modified in the pull request, rather than the existing policy in the repository.
Fixes #362
I looked for tests that exercise this code but couldn't find them. What is the best way to test this?