DNN Platform Documentation Fixes #4115
Conversation
Co-authored-by: Daniel Valadas <[email protected]>
|
@mitchelsellers I'm curious if the rules around merging pull requests with only one approval was bypassed for any specific reason. I've seen quite a few pull requests approved with only a single approval, or even merged by the author, which isn't allowed but it seems to be ignored as of late. |
|
@ohine for the protected branches (develop & master) the only PR's that should be getting merged without 2 approvals are those in support of release management activities, typically done by myself of @valadas. This does include the creation of release branches, merges to master to trigger releases & RC's and related items. For this particular PR, I'm not 100% sure what happened but I will follow-up with @donker. As a documentation update, I'm not as concerned, but we should be following processes. We are using Mergeable to enforce most of those rules, so items getting by without 2 approvals should be a very limited situation. We do have a few "special branches" out there that are not following these rules, such as the one for the updated file manager. I am contemplating further updates to remove the "Not Merged by Submitter" rule as there is a bit of practicality that makes that process a bit harder to manage considering the number of approvers, required approvers, and who helps to manage/flow the releases. If we have issues here that I've missed (which we have a LOT of activity) I'll gladly review PR's of concern. As with the initial rules we are not |
|
The irony does not escape me on this 😆 - great catch @ohine!!! |
|
My suggestion above was discussed by voice and I vocally approved, I forgot to come by here and approve it. As for this being a many pull request, many of them are for the new file manager and we agreed in meetings this would be merged quick with a single approval so we can move forward and since this is a feature branch, we are just a few collaborating on, it is acceptable since it will come in as another PR to merge into our actual develop branch. I don't see that this is a concern and a habit on master and develop the only ones I am aware where merged with a single approval are PRs related to release management as I believe was your idea @ohine |
I figured it was something along those lines, but without the transparency and just seeing items merged without full approval is a huge issues for security and the integrity of the project, and when the owner themselves merge (which isn't the case with this PR), that's an even bigger red flag. I've looked a few times for meeting notes, and I believe they're not public anymore? or if they are they're in a very obscure place.
Correct, release management tasks are a totally different story. I sat around for days waiting and pinging people begging for approvals back when I was part of the team and that caused releases and RCs to be delayed. Each time however, I've seen the disclosure as to why the policy was being bypassed. That transparency piece is huge. |
I really don't recommend that one bit, as stated above, allowing the PR owner to merge their own pull requests and when they also have the security rights to bypass the approval chain is very very dangerous. This shouldn't be removed under any circumstance and should be enforced on the first violation. |
While I can truly understand the Ivory Tower approach to this, and we do strive to follow the process as much as we can. As you have noted, we have a limited number of approvers and wrangling those approvers together in coordination can be very hard. With current live/COVID situations this is exacerbated. I do believe that 2 approvals is an absolute, after 2 unique approvals, merging by the initial submitter with those approvals although not great isn't catastrophic. Merging your own, without 2 unique approvals, that would be a different story. |
When approvers have the security rights to bypass the required approval chain and the system also allows them to self merge their own changes, it's a huge security risk. That's why requiring multiple approvals and requiring a 3rd party to merge was put into place. This helps prevent anything malicious from getting snuck into the platform. This allows the reviewers time to actually review changes. Violations to these policy's shouldn't just be ignored, as it puts the entire ecosystem at state of risk. |
|
@ohine to reiterate, this PR
I do agree here with @mitchelsellers that although not ideal as long as we have 2 approvers, merging by the initial submitter (which by the way is not any random person but is also a reviewer) is not something we do recommend but at the same time, who clicks the button does not mean much, it does not change the number of approvers. We only have 6 approvers that where chosen very carefully, that are all very busy and spend a lot of their very limited free time maintaining this project for free. At the same time, most of the contributions also come from these 6 approvers, which only leaves 5 other busy individuals to actually review.
Correct. If your concern is security and you do believe you saw any other PR that got merged without 2 approvals that does not fall in these situation:
Please point that out for double-check. If you cannot find any, then this discussion is moot, we are all in agreement here about our policy and we all care a lot about the platform security. |
Correct, I was one of those original approvers. I helped craft these rules with @mitchelsellers and the original group of code reviewers back from the corp days. I was one of the top contributors for multiple years, and I've jumped through all those hoops, multiple times. I also stood up for both you and @david-poindexter to get into the group after quite a few internal discussions as we were struggling with the workload and we desperately need additional help. As always, I'm willing to jump in and help out, but it appears that my help is just completely unwanted from the team. However, when it comes to the platform code, and the policies around who should be merging changes, and when those policies can be skipped, is of upmost importance. While most documentation changes are trivial, and do not require a strict policy, as they're documentation, this PR addresses the code review policy and the list of approvers, which should require a bit more than a single review. All I'm asking for is transparency. It takes 2 seconds to type why you're checking the admin privileges' required checkbox to bypass the required 2 approval merge policy. Providing transparency is important, as is being accountable when issues arise.
I've sent @mitchelsellers quite a few of these troubling pull requests over the years, each time I was told it will be handled and it wouldn't happen again. Yet it does. Continually. This was before the feature branches were even allowed too. I wasn't aware that the policy didn't apply to feature branches, as the policy wasn't current. I'm raising the concern here, because Mitch said he was thinking about relaxing that policy even further, which is why I continued the discussion on this thread. If you need me to dig back through the history and provide links again this issue to the pull requests which bypassed this policy since it was instated, I can, it'll just take some time. It's not an easy thing to query. That's why I'd always send them directly to Mitch as I was reviewing pull requests, which stopped about a year ago when my access was revoked. |
Honestly, I'm not getting into yet another war, especially one in a public channel where there continue to be undertones of personal vendettas. I will confirm that ONCE I was provided an example and it was discussed and reviewed. You have continually accused this abuse of power, yet after asking for the examples they were never provided. So yes, I'd love a listing of them. |
Just to be perfectly clear, in public, and on the record. I never once started any war. I don't think of this as a war. I never had. I don't have personal vendettas. It's pretty sad that I'm still getting attacked when the only thing I care about is the security of the platform and it's source code. That's it. I know for a fact I've sent you MULTIPLE, as you requested that I resend them before I was removed from the approvers groups. I will dig that thread back up and post it here, and I'll review the pull requests since I left to see what else might of occurred. I've only reviewed the major ones that have a direct effect on our clients sites, but I'd be happy to dig back in and provide the MULTIPLE examples yet again in a public channel if that's how you'd like to proceed. |
|
This isn’t something that has to be big in public, but if you have examples send them. As I informed you I did not receive before. Feel free to copy the other approvers on that email as I know you know how to reach us all. |
Actually, It needs to be public, because my name is continually dragged into the mud. I know you did receive them because it was a direct chat on Skype. We had a full conversation about it, multiple times. |
Agreed, please do provide links to the PRs you mention for the team to double-check publicly.
If you do believe anyone has been dragging your name in the mud, please do report that content, we abide to the .NET Foundation Code of Conduct https://dotnetfoundation.org/about/code-of-conduct |
Summary
This PR is a clean up of various bits of documentation included in the repositories.