Add a simulate page #455
Add a simulate page #455
Conversation
This allows a user to: - Input a branch that has a policy file. The simulate page will evaluate that policy file on the given PR - In put a user to simulate a fake approval for. The simulate page will evaluate the PR as if the user had submitted an approval - Or both! Abstracted away the majority of functionality into a BaseDetails handler. This allows consumers to submit a policy and a user to override the defaults done by the Details handler. The Details page still exists since it currently sends the information back to GitHub.
|
Could you add a screenshot of what the simulate form and the button look like to the PR description? |
This makes it possible to handle errors similar to how it was done before
| let simulatePath = currentPath.replace("details", "simulate") | ||
| document.location = simulatePath | ||
| } | ||
| document.getElementById(buttonId).addEventListener("click", redirectToSimulate) |
There was a problem hiding this comment.
I don't think this needs to be a JS function. We should be able to use a link that we style to look like a button, where the target URL is rendered as part of the page.
| prefillParam(BRANCH) | ||
| prefillParam(USERNAME) |
There was a problem hiding this comment.
Instead of hardcoding specific fields here, it might be nice to make this a bit more generic. One idea is to add a data-queryparam attribute to all of the input tags that need to be pre-filled. Then the JS can use querySelectorAll to find all of them, read the value of of the attribute to get the parameter name, and then do the logic you have here to set the value to the right thing.
| func (b *Base) EvaluateConfig(ctx context.Context, prctx pull.Context, client *github.Client, evaluator common.Evaluator, fc *FetchedConfig) common.Result { | ||
| result := evaluator.Evaluate(ctx, prctx) | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
I'm not sure we need this function since it only calls a function on an argument with no other processing.
There was a problem hiding this comment.
I went with this approach to contrast with EvaluateFetchedConfig.
This one does the evaluating portion (and any processing if required) while EvaluateFetchedConfig evaluates and posts the results. This way, the simulate page can display what is happening without messing with the status of the PR.
| logger := zerolog.Ctx(ctx) | ||
| statusDescription := result.StatusDescription |
There was a problem hiding this comment.
Any reason this moved up here instead of staying where it was?
There was a problem hiding this comment.
It made it easier to understand what was going on IMO
server/handler/base.go
Outdated
| return result | ||
| } | ||
|
|
||
| func (b *Base) PostEvaluatedResults(ctx context.Context, prctx pull.Context, client *github.Client, evaluator common.Evaluator, fc *FetchedConfig, result common.Result) (common.Result, error) { |
There was a problem hiding this comment.
Let's remove evaluator as an argument here since it is not used
server/handler/simulate.go
Outdated
| DetailsBase | ||
| } | ||
|
|
||
| type fakePrContext struct { |
There was a problem hiding this comment.
nit: let's call this SimulatePullContext or something like that
server/handler/simulate.go
Outdated
| return reviews, err | ||
| } | ||
|
|
||
| func new(ctx pull.Context) *fakePrContext { |
There was a problem hiding this comment.
nit: calling this new shadows a builtin Go function; NewSimulatePullContext or something like that would be better
server/templates/details.html.tmpl
Outdated
| @@ -1,12 +1,14 @@ | |||
| {{/* templatetree:extends page.html.tmpl */}} | |||
| {{/* templatetree:extends base-details.html.tmpl */}} | |||
| {{define "title"}}{{.PullRequest.GetBase.GetRepo.GetFullName}}#{{.PullRequest.GetNumber}} - Details | PolicyBot{{end}} | |||
There was a problem hiding this comment.
This is now duplicated with base-details.html.tmpl, as is body-class and some other templates. If we're going to extend a template, let's either remove these overrides or remove the definitions from the parent.
| {{end}} | ||
| {{define "footer"}} | ||
| <footer class="bg-white shadow-sm w-full z-10"> | ||
| <button id="details-to-simulate" class="bg-blue3 hover:bg-blue5 text-white font-bold py-2 px-4 rounded focus:outline-none focus:shadow-outline" type="button">Simulate changes on this PR</button> |
There was a problem hiding this comment.
As mentioned on the JS file, this should be a regular link that we style to look like a button if you want
| <div class="bg-light-gray3 border-b border-gray3"> | ||
| <div class="pl-5 pt-5"> | ||
| <form class="w-full max-w-md" method="get"> | ||
| <div class="md:flex md:items-center mb-6"> |
There was a problem hiding this comment.
As a general rule, I've tried to avoid the media query based classes (md: prefix) in favor of things that naturally scale and adjust as the screen size changes. I'd probably see how this works with the un-prefixed classes and then adjust from there.
|
#476 is going to conflict with this, but it should make it much easier to evaluate a PR without posting status checks. |
|
Addressed some PR comments and fixed what I could fix without getting a whole test setup. I'll be back with more questions soon enough. |
|
@bluekeyes this came up when deploying Policy-Bot. Could we get this merged? Use case was simply trying to test out a new policy on a branch, and not knowing the right path to do so. |
This allows a user to:
that policy file on the given PR
evaluate the PR as if the user had submitted an approval
Abstracted away the majority of functionality into a BaseDetails
handler. This allows consumers to submit a policy and a user to override
the defaults done by the Details handler. The Details page still exists
since it currently sends the information back to GitHub.
Here is a sample of the simulate page on a test repository:
