-
Notifications
You must be signed in to change notification settings - Fork 182
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
Allow user Regex and Allow Group prefixes #29
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
func (s *syncGSuite) allowPattern(name string) bool { | ||
if len(s.cfg.AllowGroups) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be testing the length of s.cfg.AllowPatterns
instead?
return true | ||
} | ||
for _, p := range s.cfg.AllowPattern { | ||
if p == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would accepting a pattern of "*"
be less accident prone than simply a blank pattern? Seems too easy for an errant comma causing everyone to match.
return true | ||
} | ||
|
||
re := regexp.MustCompile(p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be a performance problem with lots of patterns compiled and applied at each user? Should we really be preprocessing the patterns once to compile them (newish to go)
@@ -156,6 +163,10 @@ func (s *syncGSuite) SyncGroups() error { | |||
continue | |||
} | |||
|
|||
if ! s.allowGroup(g.Email) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit This one place you've got a blank
after the bang !
... maybe delete that?
if len(s.cfg.AllowGroups) == 0 { | ||
return true | ||
} | ||
for _, p := range s.cfg.AllowPattern { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit Everywhere else you've inserted a blank line before the for
, maybe insert one here too?
Thank you for you contribution. I'm currently doing some heavy refactoring of the core syncing functions to enable a variety of feature requests. As part of that work I've already implemented this capability. If you (and anyone else on this thread) would be interested let me know and I'll share a candidate build once I have one. |
Issue #, if available: #28
Description of changes: Added the ability to: Filter by Group prefixes and filter by email pattern.
As these are the first lines of go, I've written, maybe someone else would like to take a look how to add these to the Lambda Environment variables?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.