-
-
Notifications
You must be signed in to change notification settings - Fork 393
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
Add whitelist for syncable owners #119
Conversation
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.
Some inconsistencies in the code. Otherwise a really awesome feature, I want to merge this once we clear up the questions.
server/user.go
Outdated
sync := syncer{ | ||
remote: remote.FromContext(c), | ||
store: store.FromContext(c), | ||
perms: store.FromContext(c), | ||
match: NamespaceFilter(config.Orgs), |
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.
Looks like OwnersWhitelist
is never used, nor the related configuration variable. Looks like you abandoned that concept and just used config.Orgs
? If I think about it, that var is probably enough
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.
Adjusted! Sorry about that, missed a few things when splitting the features up into separate PRs. Will go over the other PRs too just to be sure I didn't miss anything else.
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.
No worries, this is stellar stuff!
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.
I feel the same about your fork! It was fantastic to find it. I was previously running drone 0.2 and in need of an upgrade went to the latest version and only learned about the unusual license (given all the code is available) when looking at the autoscaling component. Looked at JenkinsX to but it seems only suited to giving demos or greenfield projects.
Found your project via your blog post detailing a similar license confusion. Happy to contribute to such a lightweight, flexible CI solution that doesn't do pricing based on seats and/or repos/builds.
Thanks for the review @laszlocph, I mixed up some different commits/iterations when splitting things up into PRs, didn't spot the errors on this one. Thanks for highlighting them will adjust. |
No description provided.