Skip to content
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

Merged
merged 2 commits into from
May 20, 2020

Conversation

imduffy15
Copy link

No description provided.

Copy link
Member

@laszlocph laszlocph left a 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.

router/middleware/config.go Outdated Show resolved Hide resolved
server/user.go Outdated
sync := syncer{
remote: remote.FromContext(c),
store: store.FromContext(c),
perms: store.FromContext(c),
match: NamespaceFilter(config.Orgs),
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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!

Copy link
Author

@imduffy15 imduffy15 May 19, 2020

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.

@imduffy15
Copy link
Author

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.

@laszlocph laszlocph merged commit 14636cc into woodpecker-ci:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants