Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Simulation API #725

Merged
merged 15 commits into from Mar 14, 2024
Merged

Conversation

atatkin
Copy link
Contributor

@atatkin atatkin commented Mar 7, 2024

This pr adds a new POST route api/simulate/:owner/:repo/:number. This route requires a GitHub bearer token with access to read the the underlying pull request.

When called, policy-bot will re-evaluate the policy of the referenced pr and return a simulated result back to the caller. The result does NOT get written back to the pull request as a STATUS.

A request body can be included specifying a number of options which can modify how the simulation runs. These should be relatively easy to extend with additional options in the future.

Today this supports:

  • Simulating new comments.
  • Simulating new reviews.
  • Simulating ignoring existing reviews and comments.
  • Simulating a change of the base branch.

The route can also be used without any options to trigger policy-bot to re-evaluate the pull request as is.

The returned simulated result does not currently contain all of the fields of the base result but it should be easy to extend as needed.

Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this and including simulation features beyond what we need for the initial use case. Overall, I think this implementation makes sense, but I had some suggestions for how the simulation options and the handler should work.

pull/simulated/context_test.go Outdated Show resolved Hide resolved
pull/simulated/options.go Outdated Show resolved Hide resolved
pull/simulated/options.go Outdated Show resolved Hide resolved
pull/simulated/options.go Outdated Show resolved Hide resolved
pull/simulated/options.go Outdated Show resolved Hide resolved
server/middleware/repo_auth.go Outdated Show resolved Hide resolved
server/middleware/repo_auth.go Outdated Show resolved Hide resolved
server/middleware/repo_auth.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
policy/simulated/context_test.go Outdated Show resolved Hide resolved
policy/simulated/context_test.go Outdated Show resolved Hide resolved
policy/simulated/context_test.go Show resolved Hide resolved
policy/simulated/options.go Outdated Show resolved Hide resolved
policy/simulated/options.go Outdated Show resolved Hide resolved
server/handler/simulate.go Outdated Show resolved Hide resolved
server/handler/simulate.go Outdated Show resolved Hide resolved
server/handler/simulate.go Outdated Show resolved Hide resolved
server/handler/simulate.go Outdated Show resolved Hide resolved
server/apierror/error.go Outdated Show resolved Hide resolved
Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the fixes, I think this is almost ready to merge. Could you please also add a small section to the README about using the new API? I think it can go in the "Testing and Debugging Policies" section.

policy/common/actor.go Outdated Show resolved Hide resolved
policy/common/actor.go Outdated Show resolved Hide resolved
policy/simulated/options.go Outdated Show resolved Hide resolved
Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more small suggestion on the Review type that I think we should address before merging, but then I think this is good to merge.

policy/simulated/options.go Outdated Show resolved Hide resolved
@bluekeyes bluekeyes merged commit a793ea3 into palantir:develop Mar 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants