Repository navigation
Conversation
Generate changelog in
|
asvoboda
changed the title
| evaluator, err := evalCtx.ParseConfig(ctx, common.TriggerReview) | ||
| // COMMENTED reviews also supply approval comments, and dismissing a review | ||
| // can remove one even when GitHub review approvals are disabled. | ||
| evaluator, err := evalCtx.ParseConfig(ctx, common.TriggerReview|common.TriggerComment) |
There was a problem hiding this comment.
Are these changes necessary for slots? I'd like to see the minimal set of changes here. If this is a bug, lets separate it into its own PR (or set of PRs)?
| return err | ||
| } | ||
|
|
||
| // Audit unused rules too, even when ParseConfig found no comment trigger. |
There was a problem hiding this comment.
Why is this necessary? Now the tampering check is removed?
| } | ||
| } | ||
| result := r.Child.Evaluate(ctx, prctx) | ||
| result.Slot = &common.SlotResult{Name: r.Name} |
There was a problem hiding this comment.
I think this means that a filled slot still renders as the implicit and wrapper, even for a single-rule fill. Is that intentional? The RFC described slot content as "flattened". Should the result take the slot's name, or should we skip the wrapper when the fill has one entry? It isn't a correctness issue, but it's the first thing users will see.
| return nil, errors.WithMessage(err, "failed to parse approval policy") | ||
| } | ||
| for name, fill := range c.Slots { | ||
| if name == "" || fill == nil { |
There was a problem hiding this comment.
The empty-name check is in both Slots.UnmarshalYAML and ParsePolicy. I'd keep one and remove the other?
| assert.Equal(t, expected, result.Status) | ||
| } | ||
| // OR may suppress the aggregate error, but retains child evidence. | ||
| assert.Error(t, result.Children[0].Children[0].Children[1].Error) |
There was a problem hiding this comment.
this is a pretty brittle assertion
There was a problem hiding this comment.
agreed changed it to find by name instead
| @@ -94,7 +94,7 @@ func TestParsePolicy(t *testing.T) { | |||
| rulesByName[r.Name] = r | |||
There was a problem hiding this comment.
I'd suggest adding a slot declaration and a filled slot to to the testing table in this file
| assert.Contains(t, err.Error(), `["a-first" "z-last"]`) | ||
| } | ||
|
|
||
| func TestSlotFillDepth(t *testing.T) { |
There was a problem hiding this comment.
most of this is already under test, I think
Adds approval slots that you can fill with rules from the same policy file. Required slots have to be filled, and multiple rules in a slot stay grouped together. The details page shows filled slots and hides empty ones.