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.

Sign up for GitHub

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

Jump to bottom

Add handler for project creates #25

Merged
merged 4 commits into from
Mar 16, 2020
Merged

Add handler for project creates #25

merged 4 commits into from
Mar 16, 2020

Conversation

thisisnotashwin
Copy link
Contributor

  • This add a mutating webhook that adds the user from the request as a member of the project if a project is created with no entries in access

Add handler for project creates
925e2b5 
- This add a mutating webhook that adds the user from the request as a member of the project if a project is created with no entries in access

Signed-off-by: Ashwin Venkatesh <avenkatesh@pivotal.io>
edwardecook
edwardecook suggested changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@edwardecook edwardecook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers for the PR. Overall it looks good, with a few fairly minor exceptions, the only one that I think really needs to be addressed is considering how to handle ServiceAccounts.

pkg/webhook/project_handler.go Outdated Show resolved Hide resolved
pkg/webhook/project_handler.go Outdated Show resolved Hide resolved
pkg/webhook/project_handler.go Outdated Show resolved Hide resolved
testhelpers/helpers.go Outdated Show resolved Hide resolved
Address PR comments
a484a8f 
Signed-off-by: Danny Joyce <djoyce@pivotal.io>
edwardecook
edwardecook suggested changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@edwardecook edwardecook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all looks good to me, apologies for missing this in the first review but it suddenly occurred to me that there isn't any acceptance level testing, it would be great to have something like https://github.com/pivotal/projects-operator/blob/bafb5279c3387e1698fddc281876549355e9d6e2/acceptance/project_list_test.go#L64-L81

@djoyahoy
Copy link
Contributor

The acceptance test just checks that the access list has one user mainly because I have no way of knowing the admin users name.

@djoyahoy djoyahoy requested a review from gmrodgers March 16, 2020 13:50
edwardecook
edwardecook approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@edwardecook edwardecook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya, that all looks good to me, apologies for the delay in getting back to you and the false start where I missed the acceptance test's absence.

@djoyahoy djoyahoy merged commit ae4afc0 into master Mar 16, 2020
@djoyahoy djoyahoy deleted the ashwin-fork branch March 16, 2020 16:35
@gmrodgers gmrodgers mentioned this pull request Mar 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djoyahoy djoyahoy djoyahoy left review comments

@edwardecook edwardecook edwardecook approved these changes

@gmrodgers gmrodgers Awaiting requested review from gmrodgers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@thisisnotashwin @djoyahoy @edwardecook @gmrodgers