4 ms·

This just reads like a prequel to "why PR reviews are useless" blog post. If a bot "approves" a PR in this fashion, it's not an approval, you've just set up a

This just reads like a prequel to "why PR reviews are useless" blog post. If a bot "approves" a PR in this fashion, it's not an approval, you've just set up a "changes to this file are not worth reviewing" rule, and handed out a green check-mark particpation award. Just let your developers force-merge such non-issue pull requests. The very essence of PR approvals is someone else looking at your masterpiece, thinking about it and pointing out the things you forgot. If you don't have time for that, be honest with yourself and let go of the mandatory approval requirement. As for the latter part, the instant PRs and merges for updates, congratulations on re-implementing a monolith while calling it a microservice. Sincerely, - A very cranky developer currently very slowly steering a carelessly reviewed codebase back on track.

avitzurel 8y ago
It's not that PR are useless at all. This library has our most important code since it's used across all apps. However, the JSON definitions are just not that important, that's a fact, we can ping engineers and have them lose focus just to get approve or we can decide to automatically approve if it's under directory "X". I would argue that we did not implement a monolith at all, we just have a faster cadence to update dependency than most. With the way this works, you can't forget anything and this is why this automation is so powerful, if you change the SDK, you know that everything using this SDK is updated with the latest code. It will either fail tests immediately or you can just move on with your life and focus on more important things
- deleted 8y ago
  [deleted]
- wickedOne 8y ago
  which means your tests have a full coverage of code and scenarios?
anonydsfsfs 8y ago
If you're practicing continuous deployment with a repo used by a lot of developers, it's important to have merge restrictions in place to prevent accidents. Github lets you require every PR have at least one approval before merging, but it only works at the level of branches, not directories or paths. This tool supports path matching, so it's a nice workaround for Github's lack of granularity.
outworlder 8y ago
> Sincerely, - A very cranky developer currently very slowly steering a carelessly reviewed codebase back on track. You must be a pleasure to work with. Anyway. Another poster already mentioned the lack of granularity of pull requests. Generalizing this problem, you could apply to anything. Require PRs to be human-approved before touching your production terraform/infrastructure as a code/whatever thing, but auto-approve after basic sanity checks if the change only affects a less-important system.
jbmsf 8y ago
I don't agree at all. (Aside: I work with the author, so I am biased. We all are.) Some relevant context: - Despite your interpretation, we believe strongly in PR review outside of this narrow area. In our business, security and appropriate controls are a $BIG_DEAL and being able to say that every PR goes through a well-defined approval process (even if part of that process is automated) carries infinitely more weight than "developers force push whenever they feel like it." - One of the benefits of auto-approval is that developers control when these kinds of changes are integrated. We thought about having a fully-automated process where changes in services automatically update this library and decided that this behavior wasn't desirable because many API changes need to be _coordinated_ with other changes. We want developers to be able to decide when to merge this kind of PR in conjuction with other PRs. (The alternative is a strong commitment to backwards compatibility at all times, which is not a constraint we're willing to impose on every developer at this time in our business. YMMV.) - I honestly don't understand your comment around re-implementing a monolith, but I'd love to talk about it more since this is a topic I have many opinions about. My take is that micro-services should interoperate via HTTP APIs and that these APIs should be automated as much as possible. We do this by having our code generate Open API (aka Swagger) definitions based on our code and publishing the resulting schema at `/api/v[N]/swagger` in a deployed service. However, we still need a way for other services to to integrate with these APIs. So far, a "monolith" client library, updated in the way this post describes, has worked quite well. I am eager to hear about alternatives.
- wickedOne 8y ago
  the bits which get a bit confusing are for example: - "developers control when these kinds of changes are integrated" - "We want developers to be able to decide when to merge this kind of PR in conjuction with other PRs" it implicates that the developer who wrote the code, controls when it gets merged without a "proper" review of course i'm unaware of your codebase and your entire workflow, but a change / addition in a swagger file implicates a change in an endpoint of your api. how do you make sure other applications / resources with the changed library as a dependency, don't run into problems when you're auto merging and auto bumping dependencies?

avitzurel 8y ago

It's not that PR are useless at all. This library has our most important code since it's used across all apps. However, the JSON definitions are just not that important, that's a fact, we can ping engineers and have them lose focus just to get approve or we can decide to automatically approve if it's under directory "X". I would argue that we did not implement a monolith at all, we just have a faster cadence to update dependency than most. With the way this works, you can't forget anything and this is why this automation is so powerful, if you change the SDK, you know that everything using this SDK is updated with the latest code. It will either fail tests immediately or you can just move on with your life and focus on more important things

deleted 8y ago
[deleted]
wickedOne 8y ago
which means your tests have a full coverage of code and scenarios?

anonydsfsfs 8y ago

If you're practicing continuous deployment with a repo used by a lot of developers, it's important to have merge restrictions in place to prevent accidents. Github lets you require every PR have at least one approval before merging, but it only works at the level of branches, not directories or paths. This tool supports path matching, so it's a nice workaround for Github's lack of granularity.

outworlder 8y ago

> Sincerely, - A very cranky developer currently very slowly steering a carelessly reviewed codebase back on track. You must be a pleasure to work with. Anyway. Another poster already mentioned the lack of granularity of pull requests. Generalizing this problem, you could apply to anything. Require PRs to be human-approved before touching your production terraform/infrastructure as a code/whatever thing, but auto-approve after basic sanity checks if the change only affects a less-important system.

jbmsf 8y ago

I don't agree at all. (Aside: I work with the author, so I am biased. We all are.) Some relevant context: - Despite your interpretation, we believe strongly in PR review outside of this narrow area. In our business, security and appropriate controls are a $BIG_DEAL and being able to say that every PR goes through a well-defined approval process (even if part of that process is automated) carries infinitely more weight than "developers force push whenever they feel like it." - One of the benefits of auto-approval is that developers control when these kinds of changes are integrated. We thought about having a fully-automated process where changes in services automatically update this library and decided that this behavior wasn't desirable because many API changes need to be _coordinated_ with other changes. We want developers to be able to decide when to merge this kind of PR in conjuction with other PRs. (The alternative is a strong commitment to backwards compatibility at all times, which is not a constraint we're willing to impose on every developer at this time in our business. YMMV.) - I honestly don't understand your comment around re-implementing a monolith, but I'd love to talk about it more since this is a topic I have many opinions about. My take is that micro-services should interoperate via HTTP APIs and that these APIs should be automated as much as possible. We do this by having our code generate Open API (aka Swagger) definitions based on our code and publishing the resulting schema at `/api/v[N]/swagger` in a deployed service. However, we still need a way for other services to to integrate with these APIs. So far, a "monolith" client library, updated in the way this post describes, has worked quite well. I am eager to hear about alternatives.

wickedOne 8y ago
the bits which get a bit confusing are for example: - "developers control when these kinds of changes are integrated" - "We want developers to be able to decide when to merge this kind of PR in conjuction with other PRs" it implicates that the developer who wrote the code, controls when it gets merged without a "proper" review of course i'm unaware of your codebase and your entire workflow, but a change / addition in a swagger file implicates a change in an endpoint of your api. how do you make sure other applications / resources with the changed library as a dependency, don't run into problems when you're auto merging and auto bumping dependencies?