Adding a details tab to policy bot rules #407

yiweny · 2022-04-08T22:54:19Z

This PR is for adding an expendable details tab to policy bot.

Here is an example of the UI. User can click on the little marker to expand and collapse the details of the rule. This can help user to easily understand why a rule is selected, as well as providing them a link to the users that can approve their PR.

The way policy bot works currently is that it receives a webhook from Github, and then finds the policy written in the repo. For each rule, it determines if it is skipped or selected based on the predicates. It also determines the status of each rule based on the approval/disapproval review the PR received.

Our change adds a struct called PredicateInfo. This struct will be produced while the code is evaluating predicate and passed to Result to get rendered in html. It has two string fields and some pointer fields.

type PredicateInfo struct {
	Name        string
	Type        PredicateInfoType
	Description string

	ContributorInfo *ContributorInfo
	FileInfo        *FileInfo
	PatternInfo     *PatternInfo
	CommitInfo      *CommitInfo
	StatusInfo      *StatusInfo
}

Because each predicate requires a different set of data to display as details, we store those in a separate struct and the PredicateInfo struct stores the pointer to that struct. The Type field indicates which pointer is not null/used and the Name field is used to generate frontend text, indicating what data is related to the Predicate.

There are 15 possible predicates. In the original code, the predicates are grouped by files. Therefore, for each group of predicates, we have a specific struct to store the information of that group.

For example, there are 4 predicates in the predicate/author.go file. We have the ContributorInfo struct that stores the data needed by the predicates in author.go. Specifically, each struct needs two types of data, first is that defines the condition to qualify/disqualify a predicate, and the second is the context of the PR used for determining the (dis)qualifications. In ContributorInfo, Organizations, Teams and Users are data used for qualification purpose and Author, Contributors are the actual values of the context of the PR.

type ContributorInfo struct {
	RequiredMembership Actors
	ActualAuthor       string
	ActualContributors []string
}

Predicate Name	Fields Needed for Qualification	Fields Needed for Context
`HasAuthorIn`	Organizations, Teams, Users	Author
`OnlyHasContributorsIn`	Organizations, Teams, Users	Contributors
`HasContributorIn`	Organizations, Teams, Users	Contributors
`AuthorIsOnlyContributor`		Author,Contributors

If a field is not needed, then we would not assign values to it, and by default in go, it would be nil.

The context field would store minimal needed data for (dis)qualification. For some predicates, when translate into logic, can be expressed as either for all or there exists. If the predicate is in the form of for all, e.g. OnlyHasContributorsIn, the original code has short circuiting so that while looping over all contributors, if the code sees an example of contributor that disqualifies the predicate, then it will return right away. In this case, the Contributors field in ContributorInfo would only store the specific contributor that doesn't qualify the predicate. Otherwise, if all contributors satisfy a condition, then the Contributors field would store all the contributors. The same short circuiting scheme applies to there exists, if there exists a value that qualifies a predicate, then we would simply return that value, ignoring other potential candidates.

To display the reviewer information, we move the Requires struct from approval/disapproval to common package. So now both approval and disapproval uses the same Requires struct. This way we can easily pass the Requires struct to Result. Previous, the Requires struct in disapproval does not have a count field. Now we are just setting that number to 1 to keep the same behavior.

For the front end part, we added two templates, result-predicate-details and result-requires-details. They are for displaying the predicate information and review information.

getPredicateInfo and getPredicateRequirement in the frontend.go file are used to render the text displayed.

getPredicateInfo is used to obtain the context information related to each predicate, specifically, the value we store in the name field. And getPredicateRequirement is for obtaining the values for qualification/disqualification. For example, in HasAuthorIn predicate, it returns the organizations, teams and users. However, this function does not cover all the predicates. For boolean predicates like HasSuccessfulStatus or AuthorIsOnlyContributor, the explanatory requirement is not needed because the name of the predicate is already explanatory.

We also added the PredicateInfo to each predicate test. There are two issues worth thinking here.

An issue is that using maps as dictionaries in the code would produce non-deterministic behaviors when testing in go. For example, in the committersInListButAuthorsAreNot test for the OnlyHasContributorsIn predicate, both ttest1 and ttest2 can be returned as result(because of short circuiting only one of the two will be returned). Therefore, my solution is to use the Subset function in the testify package. However, this cause us to not test equality in the other cases where there is no short circuiting. I do not know what is the best approach to this but two ways I can think of is 1) simply change the code to not use non-deterministic data structures like maps. 2) remove the test of which the result is non-deterministic.

Another issue is related to the 'labels do not exist' test in label_test.go. I am not sure the point of testing it. I assume when labels do not exist then there is an error, so we can just return nil for the PredicateInfo. Are we just testing a case of error?

bluekeyes · 2022-04-12T19:22:38Z

An issue is that using maps as dictionaries in the code would produce non-deterministic behaviors when testing in go. [...] I do not know what is the best approach to this but two ways I can think of is 1) simply change the code to not use non-deterministic data structures like maps. 2) remove the test of which the result is non-deterministic.

Now that it matters, I suggest extracting the keys from the map, sorting them, and then iterating through the sorted list in the predicate implementation. We generally still need to use a map because we need a data structure that functions like a set and using map is the idiomatic way to do this in Go. Sorting the keys as a list is a good way to make the results deterministic.

Another issue is related to the 'labels do not exist' test in label_test.go. I am not sure the point of testing it. I assume when labels do not exist then there is an error, so we can just return nil for the PredicateInfo. Are we just testing a case of error?

It looks like this was added to test the difference between a nil value and a []string{} value. For most uses, these are equivalent in Go, so I don't think this test is adding much value. It should be safe to delete.

bluekeyes

This looks like a good start! In this initial review, I left some higher-level comments about the overall structure and design, which I'm happy to talk more about when we meet tomorrow. I think we'll also want to look at the visual design, but that's hard to communicate here, so I'll try to make a mockup or drawing.

Later, I'll do a second review looking more at details and style things, but I didn't want to comment on code that might be deleted if the structure changes. But if you prefer, I can do that now so you have everything at once.

policy/common/predicateinfo.go

policy/approval/approve_test.go

policy/common/result.go

policy/common/predicateinfo.go

policy/predicate/file.go

server/handler/frontend.go

bluekeyes

Thanks for the updates! Looking through this again, I had another idea for how to further simplify the PredicateInfo concept, hopefully making this easier to understand and extend in the future. Let me know if it make sense to you or if you have any disagreements or questions. I think it can work in all cases, but I haven't actually tried every single predicate.

That's really the only design comment I have, although it is a big one. Look for the comments that start with (1/?) for discussion on that topic. Everything else is relatively minor comments about code style.

I appreciate you iterating with me on this so far!

bluekeyes · 2022-04-18T18:56:55Z

policy/approval/approve.go

+	res.Requires = common.Actors{
+		Organizations: r.Requires.Organizations,
+		Teams:         r.Requires.Teams,
+		Users:         r.Requires.Users,
+	}


Suggested change

res.Requires = common.Actors{

Organizations: r.Requires.Organizations,

Teams: r.Requires.Teams,

Users: r.Requires.Users,

}

res.Requires = r.Requires.Actors

Embedded fields in Go are implicitly named after their type, so can use this to copy the whole field in one operation.

bluekeyes · 2022-04-18T19:00:07Z

policy/approval/approve.go

+		Users:         r.Requires.Users,
+	}
+
+	var predicatesInfo []*common.PredicateInfo


style: predicateInfos

bluekeyes · 2022-04-18T19:00:31Z

policy/approval/approve.go


 	for _, p := range r.Predicates.Predicates() {
-		satisfied, desc, err := p.Evaluate(ctx, prctx)
+		satisfied, pPredicateInfo, err := p.Evaluate(ctx, prctx)


style: rename pPredicateInfo to info

policy/common/result.go

bluekeyes · 2022-04-18T19:08:49Z

policy/disapproval/disapprove.go

+	res.Requires = common.Actors{
+		Organizations: p.Requires.Organizations,
+		Teams:         p.Requires.Teams,
+		Users:         p.Requires.Users,
+	}


Suggested change

res.Requires = common.Actors{

Organizations: p.Requires.Organizations,

Teams: p.Requires.Teams,

Users: p.Requires.Users,

}

res.Requires = p.Requires.Actors

bluekeyes · 2022-04-19T21:09:57Z

policy/predicate/author.go

+		Type:            common.ContributorType,
+		Name:            "HasAuthorIn",
+		ContributorInfo: &contributorInfo,
+	}


(4/?) To give an example of how the proposed PredicateInfo struct might work in a predicate:

info := PredicateInfo{ ValuesPhrase: "authors", Values: []string{author}, ConditionPhrase: "meet the required membership conditions", ConditionsMap: map[string][]string{ "Organizations": pred.Organizations, "Teams": pred.Teams, "Users": pred.Users, }, }

bluekeyes · 2022-04-19T21:23:21Z

policy/predicate/author.go

+		Type:            common.ContributorType,
+		Name:            "AuthorIsOnlyContributor",
+		ContributorInfo: &contributorInfo,
+	}


(5/?) Here's another example:

info := PredicateInfo{ ValuesPhrase: "commits", Values: []string{fmt.Sprintf("%.10s", c.SHA}, // or the full list of commit SHAs ConditionPhrase: "meet the authorship requirement", ConditionValues: []string{ fmt.Sprintf("authored and committed by %s", author), }, }

bluekeyes · 2022-04-19T21:24:22Z

policy/predicate/file.go

@@ -140,6 +192,14 @@ func (exp ComparisonExpr) MarshalText() ([]byte, error) {
 	return []byte(fmt.Sprintf("%s %d", op, exp.Value)), nil
 }

+func (exp ComparisonExpr) MarshalTextToString() string {


This should be called String to match Go conventions (implement the Stringer interface)

bluekeyes · 2022-04-19T21:27:43Z

policy/predicate/file.go

+func (exp ComparisonExpr) MarshalTextToString() string {
+	res, err := exp.MarshalText()
+	if err != nil {
+		return "Unknown"


Suggested change

return "Unknown"

return fmt.Sprintf("?? (op:%d) %d"), exp.Op, exp.Value)

This will give at least a bit of information about what's wrong.

bluekeyes · 2022-04-19T21:31:40Z

policy/predicate/file.go

+	fileInfo.TotalLimit = string(res[:])
+	fileInfo.TotalModifiedLines = additions + deletions
+	predicateInfo.Description = fmt.Sprintf("modification of (+%d, -%d) does not match any conditions", additions, deletions)
+	return false, &predicateInfo, nil


(6/?) One more example

info := PredicateInfo{ ValuesPhrase: "file modifications", Values: []string{fmt.Sprintf("+%d / -%d (total %d)", additions, deletions, additions+deletions)}, ConditionPhrase: "meet the modification conditions", ConditionValues: []string{ // filter this to only contain the non-empty conditions fmt.Sprintf("added %s", pred.Additions), fmt.Sprintf("deleted %s", pred.Deletions), fmt.Sprintf("total %s", pred.Total), } }

bluekeyes

Sorry for the flood of comments, but from a code perspective this looks good! Thanks for implementing the PredicateResult changes. Almost all of my suggestions are minor things to improve the phrasing of messages. There are also a couple style things that would be good to address.

Beyond that, we can talk about any CSS changes tomorrow and then I think this will be ready to merge.

There's a few more things we could do to make the messages read even better, but I think the current implementation (with the suggestions in this review) is good enough for an initial version, so I don't want to delay this any further.

bluekeyes · 2022-04-25T20:03:29Z

server/templates/details.html.tmpl

+  <ul class="list-decimal list-inside py-2">
+  {{range .PredicateResults}}
+  <li>
+  The {{.ValuePhrase}} :


Suggested change

The {{.ValuePhrase}} :

The {{.ValuePhrase}}:

bluekeyes · 2022-04-25T20:04:10Z

server/templates/details.html.tmpl

+  <ul class="list-none list-inside">
+  {{range $key, $value := getRequires .}}
+    <li class="p-2 text-sm">
+    <p> {{$key}} : </p>
+      <ul class="list-disc list-inside">
+      {{range $value}}
+        <li class="ml-4">
+          <a href={{.Link}}>{{.Name}}</a>
+        </li>
+      {{end}}
+      </ul>
+    </li>
+  {{end}}
+  </ul>


Did you try using the descriptions lists here as well, instead of the p tag with the key?

bluekeyes · 2022-04-25T20:05:17Z

server/handler/frontend.go

+
+	}
+	for _, user := range result.Requires.Users {
+		membershipInfo["Users"] = append(membershipInfo["Users"], Membership{Name: user, Link: githubURL + user})


Suggested change

membershipInfo["Users"] = append(membershipInfo["Users"], Membership{Name: user, Link: githubURL + user})

membershipInfo["Users"] = append(membershipInfo["Users"], Membership{Name: user, Link: githubURL + "/" + user})

bluekeyes · 2022-04-25T20:05:48Z

server/handler/frontend.go

@@ -55,6 +60,10 @@ func LoadTemplates(c *FilesConfig, basePath string) (templatetree.HTMLTree, erro

 			return r
 		},
+		"isRequiresNotEmpty": isRequiresNotEmpty,


style: I might call this hasRequires

bluekeyes · 2022-04-25T20:10:44Z

policy/predicate/author.go

+	predicateResult := common.PredicateResult{
+		Satisfied:       result,
+		Description:     desc,
+		ValuePhrase:     "Author",


Suggested change

ValuePhrase: "Author",

ValuePhrase: "authors",

I know there's only one author, but this will fit better with the list and the rest of the phrasing.

bluekeyes · 2022-04-25T22:22:43Z

policy/predicate/signature.go

@@ -129,11 +191,17 @@ func (pred *HasValidSignaturesByKeys) Evaluate(ctx context.Context, prctx pull.C
 			}
 		}
 		if !isValidKey {
-			return false, fmt.Sprintf("Key %q does not meet the required key conditions for signing", key), nil
+			predicateResult.Values = []string{key}
+			predicateResult.ValuePhrase = "key"


Suggested change

predicateResult.ValuePhrase = "key"

predicateResult.ValuePhrase = "keys"

predicateResult.ConditionPhrase = "exist in the set of allowed keys"

bluekeyes · 2022-04-25T22:24:45Z

policy/predicate/status.go

+		ValuePhrase:     "check",
+		ConditionPhrase: "has successful status",


Suggested change

ValuePhrase: "check",

ConditionPhrase: "has successful status",

ValuePhrase: "status checks",

ConditionPhrase: "exist and pass",

Here's another place that doesn't set ConditionMap or ConditionValues

policy/predicate/status.go

bluekeyes · 2022-04-25T22:27:15Z

policy/predicate/title.go

+		ConditionPhrase: "meet the pattern requirement",
+	}
+
+	var MatchPatterns, NotMatchPatterns []string


style: local variables start with lowercase letters by convention (matchPatterns, notMatchPatterns)

bluekeyes · 2022-04-25T22:28:21Z

policy/predicate/title.go

 	title := prctx.Title()

+	predicateResult := common.PredicateResult{
+		Satisfied:       false,
+		ValuePhrase:     "title",


Suggested change

ValuePhrase: "title",

ValuePhrase: "titles",

shouldn't there be only 1 title for each pr?

bluekeyes

Thanks for working through all the iterations here, this is looking good! I spotted two things that I think are unused and had some minor suggestions on the CSS (although I haven't tested the changes.) Once those are fixed, I think this is ready to merge.

bluekeyes · 2022-04-27T20:50:42Z

policy/predicate/signature.go

 		}
 	}

+	var keyList []string


Should this be used somewhere? Right now, it looks like you only append to it and never use the resulting list.

Yeah. Previously I returned both keys and the commit hashes. Since now we only want commit hashes, this should be removed. Good catch.

bluekeyes · 2022-04-27T20:53:18Z

server/assets/css/main.css

+.list-numbers > li {
+  @apply ml-2;
+}
+
+.list-star > li:before {
+  content: "*";
+}
+


I think both of these rules are unused now

bluekeyes · 2022-04-27T20:58:55Z

server/handler/frontend.go

@@ -73,3 +82,26 @@ func Static(prefix string, c *FilesConfig) http.Handler {

 	return http.StripPrefix(prefix, http.FileServer(http.Dir(dir)))
 }
+
+func hasRequires(result common.Actors) bool {
+	if len(result.Organizations) != 0 || len(result.Teams) != 0 || len(result.Users) != 0 {


I meant to comment on this earlier, but forgot - common.Actors can also select users by their permissions level, which is a condition we're ignoring at the moment.

I don't think it should be part of this PR, but it would be nice to make a followup change to add minimal support for rules that use permissions. I don't think it's common in the policies you have been using for testing but is pretty common in other policies.

When we do add that support, I think you can replace this function with the existing IsEmpty() method on common.Actors.

bluekeyes · 2022-04-27T21:02:41Z

server/templates/details.html.tmpl

+      <div class="pt-2"></div>
+      <div class="border-t border-light-gray3">


I think you should be able to combine these divs by adding mt-2 to the div with the border. I like to avoid empty div when I can, and it should be possible here.

bluekeyes · 2022-04-27T21:03:00Z

server/templates/details.html.tmpl

+      <div class="pt-2"></div>
+      <div class="border-t border-light-gray3">
+       {{if .PredicateResults}}
+          <div class="pt-2 mt-2">


I think this only needs pt-2 now that the border is defined on a different element. That will also match the requires div below.

backend changes

e4407ba

yiweny force-pushed the yyuan/add-rule-details branch from 37638aa to e4407ba Compare April 8, 2022 22:56

Yiwen Yuan added 3 commits April 10, 2022 18:01

added a bunch of tests

e3f6874

formatting

64f02c6

some minor fixes to display signature related predicates

eb6c9a4

yiweny force-pushed the yyuan/add-rule-details branch from fed0e44 to c0661fd Compare April 11, 2022 20:52

yiweny marked this pull request as ready for review April 11, 2022 20:54

yiweny requested review from IainSteers, bluekeyes and derekjobst April 11, 2022 21:11

fix style and remove unused code

ca1873a

yiweny force-pushed the yyuan/add-rule-details branch from c0661fd to ca1873a Compare April 12, 2022 16:51

bluekeyes requested changes Apr 12, 2022

View reviewed changes

remove desc field

d6627ea

yiweny force-pushed the yyuan/add-rule-details branch 2 times, most recently from a9ff676 to 4c94438 Compare April 18, 2022 17:14

bluekeyes requested changes Apr 19, 2022

View reviewed changes

yiweny force-pushed the yyuan/add-rule-details branch 2 times, most recently from 2c06e95 to c1c287d Compare April 22, 2022 23:25

bluekeyes requested changes Apr 25, 2022

View reviewed changes

yiweny force-pushed the yyuan/add-rule-details branch 6 times, most recently from 405cdb6 to 9b52479 Compare April 27, 2022 00:08

bluekeyes reviewed Apr 27, 2022

View reviewed changes

Yiwen Yuan and others added 2 commits April 27, 2022 14:36

fix the code based on review comments

5bf42c3

Update some styles for the details element

551cf4f

fixing css and related issues based on review comments

7cbede3

yiweny force-pushed the yyuan/add-rule-details branch from 9b52479 to 7cbede3 Compare April 27, 2022 21:37

bluekeyes approved these changes Apr 27, 2022

View reviewed changes

bluekeyes merged commit d51a125 into develop Apr 27, 2022

bluekeyes deleted the yyuan/add-rule-details branch April 27, 2022 21:49

This was referenced May 2, 2022

FR: Make it easier to tell which files required particular owners #343

Closed

FR: UI should give links to Github users and teams that can approve specific checks #368

Closed

FR: UI should expand which teams (and users) can actually approve something #49

Closed

derekjobst mentioned this pull request May 3, 2022

Fix UI for rules with required review by permissions #416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a details tab to policy bot rules #407

Adding a details tab to policy bot rules #407

yiweny commented Apr 8, 2022 •

edited

Loading

bluekeyes commented Apr 12, 2022

bluekeyes left a comment

bluekeyes left a comment

bluekeyes Apr 18, 2022

bluekeyes Apr 18, 2022

bluekeyes Apr 18, 2022

bluekeyes Apr 18, 2022

bluekeyes Apr 19, 2022

bluekeyes Apr 19, 2022

bluekeyes Apr 19, 2022

bluekeyes Apr 19, 2022

bluekeyes Apr 19, 2022

bluekeyes left a comment

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

bluekeyes Apr 25, 2022

yiweny Apr 26, 2022

bluekeyes left a comment

bluekeyes Apr 27, 2022

yiweny Apr 27, 2022

bluekeyes Apr 27, 2022

bluekeyes Apr 27, 2022

bluekeyes Apr 27, 2022

bluekeyes Apr 27, 2022

	return "Unknown"
	return fmt.Sprintf("?? (op:%d) %d"), exp.Op, exp.Value)

	membershipInfo["Users"] = append(membershipInfo["Users"], Membership{Name: user, Link: githubURL + user})
	membershipInfo["Users"] = append(membershipInfo["Users"], Membership{Name: user, Link: githubURL + "/" + user})

	predicateResult.ValuePhrase = "key"
	predicateResult.ValuePhrase = "keys"
	predicateResult.ConditionPhrase = "exist in the set of allowed keys"

		ValuePhrase: "check",
		ConditionPhrase: "has successful status",

		<div class="pt-2"></div>
		<div class="border-t border-light-gray3">

Adding a details tab to policy bot rules #407

Adding a details tab to policy bot rules #407

Conversation

yiweny commented Apr 8, 2022 • edited Loading

bluekeyes commented Apr 12, 2022

bluekeyes left a comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiweny commented Apr 8, 2022 •

edited

Loading