5 ms·

I can create a PR, get it reviewed / approved, then change 100% of the underlying code and still merge it. Having required reviews is a step in the right direc

by web007 10y ago

I can create a PR, get it reviewed / approved, then change 100% of the underlying code and still merge it. Having required reviews is a step in the right direction, but not quite good enough to work as a proper code review / signoff for strict compliance requirements.

RSZC 10y ago
You can automate such a check via the github api. Resetting approval status on every commit is probably not desired behavior for most orgs.
- Nursie 10y ago
  It is in the bank I work with right now. Makes sense to me too...
- mugsie 10y ago
  but the code could completely change in the next revision ... making the review pointless.
  - stepanhruda 10y ago
    It could but most orgs can assume good actors, and always resetting would slow the process down.
    - mugsie 10y ago
      Not resetting would seem to make the review process moot. If you want speed, just allow everyone to merge
      - stepanhruda 10y ago
        No, you want a review, but can trust that people will ask for re-review if they make significant changes. If you want to completely stop people from comitting malicious code, resetting the review process is a weak safeguard.
        mugsie 10y ago
        eh ... if you want to audit code being committed (and for open source projects - you really do, or if you have SOX compliance regulations, or other audit processes) it is one of the best safeguards. I have seen many many bugs being introduced as part of a fix for a review comment. This is why you re-review.
TheAceOfHearts 10y ago
Could you expand on your use-case? I'm unfamiliar with this kind of scenario and I'd like to learn more about your constraints and their reason. Maybe I'm being completely naïve here, but it would seem like anyone with very strict compliance requirements wouldn't be using GitHub in the first place. Is this line of reasoning wrong? At least for my small (5-ish people) team, it's been great. It makes it much easier to communicate the current status. But since we're such a small team, we largely just trust everyone to be careful. We generally don't commit directly to master and all PRs are reviewed. But if there's a good reason to commit directly or immediately merge a PR without review, we're fine with that. We'll usually just notify the team on Slack and they'll find out in the morning. For example: we get a bug report and there's a quick fix, but nobody is around to review it. The developer is encouraged to merge, confirm it's working on staging, and release it. Being able to release multiple times per day might not seem like a big deal, but it means you start releasing smaller changes with increased frequency, so even if something goes wrong, you can easily revert and you know exactly what to investigate.
- rubidium 10y ago
  Not OP, but here's some extra input (from an engineers, not quality persons standpoint). Standard ISO9000 requires approval process for all design outputs. Anything changed after approval would need to be "red-lined" and even then still approved. 3 approaches: 1) Don't make your Code the output, make the program the output. Then have automated testing to verify meets performance. 2) Make code your output, but lump all approvals together before release essentially decoupling code submitting and approvals 3) if you're a continuously deploying, strict compliance company, you may be stuck with approval required on every code line change. There's ways around that... but it depends on your compliance requirements and staff's level of competence.
- web007 10y ago
  For ISO / PCI / SOX / HIPAA / contractual compliance in any "real" company there is a requirement of a review on all code shipped to production. "Real" because it's a huge pain in the ass, and you only do it if you have to work with another such company or if you're constrained by regulation to do so. It doesn't matter the number of commiters, size of changes or frequency of release. You're bound to a process for reasons outside your control. That's the strict version that I'd like them to implement, and what I thought we got out of enabling required reviews before reading in more detail. Even for one of their use-cases as described their flow won't work: allowing review of stylistic change to match project requirements. As it stands you can approve a change, then someone can screw with tabs-vs-spaces or reformat and merge on their own. Even rebasing to squash or reorganize doesn't require a subsequent approval. As suggested elsewhere, you'd have to automate the check (good idea anyway) and force it as a required test vs using approvals anyway. Two things would improve the process. One would be implementing the strict mode, where merges can only happen after an approved review and any new commit (or even rebase) would trigger a reset. The second option would be to force two-man rule, where the committer can't merge their own PR. That fixes the compliance piece, and that's probably technically easier but logistically more complicated; person A can open a PR, then person B can approve AND make changes on the branch and still submit. That second flow is probably fine for OSS stuff, but is already covered by public repos and only allowing owners to merge. For a team it's kind of half-and-half, it depends on your requirements if it will pass compliance muster.
hhjkjhkjhhlkj 10y ago
This also made me sad. I approved a specific state. If it has changed then my approval should not be assumed. Still it is a start, and will hopefully improve.

by web007 10y ago

RSZC 10y ago

You can automate such a check via the github api. Resetting approval status on every commit is probably not desired behavior for most orgs.

Nursie 10y ago
It is in the bank I work with right now. Makes sense to me too...
mugsie 10y ago
but the code could completely change in the next revision ... making the review pointless.
- stepanhruda 10y ago
  It could but most orgs can assume good actors, and always resetting would slow the process down.
  - mugsie 10y ago
    Not resetting would seem to make the review process moot. If you want speed, just allow everyone to merge
    - stepanhruda 10y ago
      No, you want a review, but can trust that people will ask for re-review if they make significant changes. If you want to completely stop people from comitting malicious code, resetting the review process is a weak safeguard.
      - mugsie 10y ago
        eh ... if you want to audit code being committed (and for open source projects - you really do, or if you have SOX compliance regulations, or other audit processes) it is one of the best safeguards. I have seen many many bugs being introduced as part of a fix for a review comment. This is why you re-review.

TheAceOfHearts 10y ago

Could you expand on your use-case? I'm unfamiliar with this kind of scenario and I'd like to learn more about your constraints and their reason. Maybe I'm being completely naïve here, but it would seem like anyone with very strict compliance requirements wouldn't be using GitHub in the first place. Is this line of reasoning wrong? At least for my small (5-ish people) team, it's been great. It makes it much easier to communicate the current status. But since we're such a small team, we largely just trust everyone to be careful. We generally don't commit directly to master and all PRs are reviewed. But if there's a good reason to commit directly or immediately merge a PR without review, we're fine with that. We'll usually just notify the team on Slack and they'll find out in the morning. For example: we get a bug report and there's a quick fix, but nobody is around to review it. The developer is encouraged to merge, confirm it's working on staging, and release it. Being able to release multiple times per day might not seem like a big deal, but it means you start releasing smaller changes with increased frequency, so even if something goes wrong, you can easily revert and you know exactly what to investigate.

rubidium 10y ago
Not OP, but here's some extra input (from an engineers, not quality persons standpoint). Standard ISO9000 requires approval process for all design outputs. Anything changed after approval would need to be "red-lined" and even then still approved. 3 approaches: 1) Don't make your Code the output, make the program the output. Then have automated testing to verify meets performance. 2) Make code your output, but lump all approvals together before release essentially decoupling code submitting and approvals 3) if you're a continuously deploying, strict compliance company, you may be stuck with approval required on every code line change. There's ways around that... but it depends on your compliance requirements and staff's level of competence.
web007 10y ago
For ISO / PCI / SOX / HIPAA / contractual compliance in any "real" company there is a requirement of a review on all code shipped to production. "Real" because it's a huge pain in the ass, and you only do it if you have to work with another such company or if you're constrained by regulation to do so. It doesn't matter the number of commiters, size of changes or frequency of release. You're bound to a process for reasons outside your control. That's the strict version that I'd like them to implement, and what I thought we got out of enabling required reviews before reading in more detail. Even for one of their use-cases as described their flow won't work: allowing review of stylistic change to match project requirements. As it stands you can approve a change, then someone can screw with tabs-vs-spaces or reformat and merge on their own. Even rebasing to squash or reorganize doesn't require a subsequent approval. As suggested elsewhere, you'd have to automate the check (good idea anyway) and force it as a required test vs using approvals anyway. Two things would improve the process. One would be implementing the strict mode, where merges can only happen after an approved review and any new commit (or even rebase) would trigger a reset. The second option would be to force two-man rule, where the committer can't merge their own PR. That fixes the compliance piece, and that's probably technically easier but logistically more complicated; person A can open a PR, then person B can approve AND make changes on the branch and still submit. That second flow is probably fine for OSS stuff, but is already covered by public repos and only allowing owners to merge. For a team it's kind of half-and-half, it depends on your requirements if it will pass compliance muster.

hhjkjhkjhhlkj 10y ago

This also made me sad. I approved a specific state. If it has changed then my approval should not be assumed. Still it is a start, and will hopefully improve.