Fix to allow whitelist globbing for strings after wildcard #796
Conversation
| "owner/prefix-repo", | ||
| "github.com", | ||
| true, | ||
| }, |
There was a problem hiding this comment.
Can you add tests for the wildcard at the beginning of the string too: *-owner/repo? That will the cover all the cases I think.
There was a problem hiding this comment.
Thanks! I added the new test case and ran the test. If you are okay let me know and I can squash & merge.
There was a problem hiding this comment.
So I meant at the beginning of the whitelist:
{
"should match if wildcard is at beginning",
"*-owner/repo",
"prefix-owner/repo",
"github.com",
true,
},There was a problem hiding this comment.
So i'm a bit confused since all of the other tests start with "github.com" as the second value in the struct (= whitelist) and I thought this was required in the whitelist as per the below statement on this doc:
https://www.runatlantis.io/docs/server-configuration.html#repo-whitelist
Format is {hostname}/{owner}/{repo}, ex. github.com/runatlantis/atlantis
There was a problem hiding this comment.
It's not required. You can do --repo-whitelist='*' for example to match everything.
Codecov Report
@@ Coverage Diff @@
## master #796 +/- ##
==========================================
- Coverage 72.78% 72.77% -0.02%
==========================================
Files 63 63
Lines 4807 4812 +5
==========================================
+ Hits 3499 3502 +3
- Misses 1050 1051 +1
- Partials 258 259 +1
Continue to review full report at Codecov.
|
| "owner/prefix-repo", | ||
| "github.com", | ||
| true, | ||
| }, |
There was a problem hiding this comment.
So I meant at the beginning of the whitelist:
{
"should match if wildcard is at beginning",
"*-owner/repo",
"prefix-owner/repo",
"github.com",
true,
},| // substr(rule): -abc | ||
| if wildcardIdx != len(rule)-1 { | ||
| // If the rule substring after wildcard does not exist in the candidate, then it is not a match. | ||
| if !strings.Contains(candidate, rule[wildcardIdx+1:]) { |
There was a problem hiding this comment.
This works, but I think it might be clearer to re-use the strings.Index (or LastIndex) call.
idx := strings.LastIndex(candidate, rule[wildcardIdx+1:])
if idx == -1 {
return false
}
return candidate[idx:] == rule[wildcardIdx+1:]For me this seems clearer because rather than relying on strings.Contains implying that strings.Index will return a positive value, we're making that requirement clear in the code.
| return false | ||
| } | ||
|
|
||
| return candidate[strings.Index(candidate, rule[wildcardIdx+1:]):] == rule[wildcardIdx+1:] |
There was a problem hiding this comment.
I think we need to use strings.LastIndex here? Otherwise this test fails:
{
"should match with duplicate",
"*runatlantis",
"runatlantis/runatlantis",
"github.com",
true,
},There was a problem hiding this comment.
is this a test you just added ad-hoc? also not understanding this test rationale (as per my other comment to include github.com at the beginning). So the reply to that one may also answer this one. thanks!
|
@lkysow thanks for responding to my questions and for taking a closer look at my code, much appreciated. i think I covered everything and I went ahead and added the other tests you referred to. If it all looks okay I can do the squash & merge. |
Fixes #692