Hacker News Reader

Comment by silversmith | Hacker News Reader

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.

View on HN

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

which means your tests have a full coverage of code and scenarios?

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.

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?

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.

Reply on news.ycombinator.com