Conversation
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
There was a problem hiding this comment.
Pull request overview
This PR optimizes scope action merging in internal/auth to reduce allocations during ScopeSet.Add() merges, and adds a benchmark to measure NewScopeSet allocation behavior.
Changes:
- Reworked
mergeAndDedupActionsto avoid allocating a combined slice when iteratinglhs+rhs. - Added
BenchmarkNewScopeSetto track allocations for common scope combinations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/auth/scopeset.go |
Refactors action merge/dedup logic to reduce allocations by appending directly to lhs. |
internal/auth/scopeset_test.go |
Adds a benchmark for NewScopeSet allocation tracking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func mergeAndDedupActions(lhs, rhs []string) []string { | ||
| seen := make(map[string]bool, len(lhs)) | ||
| for _, elem := range lhs { | ||
| seen[elem] = true | ||
| } |
There was a problem hiding this comment.
mergeAndDedupActions no longer actually deduplicates elements already present in lhs. The previous implementation removed duplicates across lhs+rhs by building a fresh result while tracking seen; the new implementation seeds seen from lhs but then returns lhs unchanged (except for appends from rhs), so any duplicates already in lhs remain. This is a behavior change that can surface if the first scope for a resource contains duplicated actions (e.g. a scope string like pull,pull). Consider rebuilding lhs in-place (or into a new slice) while checking seen for lhs entries as well, to preserve the original dedup semantics while still avoiding the append(lhs, rhs...) allocation.
No description provided.