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

Document security reviewer conflict of interest #247

Merged
merged 5 commits into from Aug 19, 2019

Conversation

lumjjb
Copy link
Collaborator

@lumjjb lumjjb commented Aug 2, 2019

To address #156

Signed-off-by: Brandon Lum lumjjb@gmail.com

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
@lumjjb lumjjb requested a review from ultrasaurus Aug 2, 2019
assessments/guide/security-reviewer.md Outdated Show resolved Hide resolved
assessments/guide/security-reviewer.md Show resolved Hide resolved
assessments/guide/security-reviewer.md Show resolved Hide resolved
assessments/guide/security-reviewer.md Outdated Show resolved Hide resolved
@lumjjb
Copy link
Collaborator Author

lumjjb commented Aug 7, 2019

From meeting 08/07: Create a process to evaluate conflicts by chairs

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
@ultrasaurus ultrasaurus requested a review from pragashj Aug 13, 2019
ultrasaurus
ultrasaurus previously approved these changes Aug 13, 2019
Copy link
Member

@ultrasaurus ultrasaurus left a comment

Previous comments have been addressed.
Adding @pragashj and @dshaw to review -- ideally we have +1 from all chairs,
but majority is sufficient

@ultrasaurus ultrasaurus requested a review from dshaw Aug 13, 2019
pragashj
pragashj previously approved these changes Aug 19, 2019
Copy link
Collaborator

@pragashj pragashj left a comment

Few minot nits, but otherwise LGTM. Thanks Sarah, for putting this together!


- Individuals from intentionally or unintentionally promote their own company's project
- SIG-Security chairs and assessment leads could intentionally or unintentionally limit the participation of an individual unfairly by asserting conflict of interest
- Security reviews being stalled while groups discuss whether this person or that person should be allowed to participate
Copy link
Collaborator

@pragashj pragashj Aug 19, 2019

Choose a reason for hiding this comment

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

Just a nit
Instead of discuss whether this person or that person how about "groups belabor on who"

security assessment documentation. It is the responsibility of the security reviewer
to make known his/her own conflict of interests.

For each project, 2 SIG-Security chairs must sign off on the conflicts presented to them that the assessment lead has no conflicts, and reviewers have no hard conflicts.
Copy link
Collaborator

@pragashj pragashj Aug 19, 2019

Choose a reason for hiding this comment

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

As much as possible we try to avoid conflict, but in cases where we need to go with a conflict, we should document the justification for allowing the conflict. This could be as specific as "This reviewer is the expert in this field and therefore we require their input" or as conventional as "No one else has the bandwidth to take this on at this time so we are letting an exception" -- over time, if we are documenting and reviewing the reason for exception it will allow us to keep the process honest.

lumjjb added 2 commits Aug 19, 2019
Signed-off-by: Brandon Lum <lumjjb@gmail.com>
@lumjjb lumjjb dismissed stale reviews from pragashj and ultrasaurus via cb086ba Aug 19, 2019
@lumjjb
Copy link
Collaborator Author

lumjjb commented Aug 19, 2019

@pragashj addressed comments!

@lumjjb lumjjb dismissed stale reviews from pragashj and ultrasaurus via cb086ba 9 minutes ago

It seems like a setting is enabled to dismiss LGTMs on code pushes? @ultrasaurus @pragashj could you review it again?

Do we want to keep this setting? Seems like it's not critical for us to ensure that reviews need to be closely up to date.

@ultrasaurus
Copy link
Member

ultrasaurus commented Aug 19, 2019

@lumjjb agreed on the setting... changed to not required. Will take a look at the changes, since it's not retractive!

Copy link
Member

@ultrasaurus ultrasaurus left a comment

nice edits from @lumjjb based on suggestions from @pragashj

@ultrasaurus ultrasaurus merged commit 5508775 into master Aug 19, 2019
1 check passed
@ultrasaurus ultrasaurus deleted the conflict-of-interest branch Aug 19, 2019
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.

None yet

5 participants