February 28, 2024 at 12:00 am
Comments posted to this topic are about the item The Art of the Code Review
February 28, 2024 at 9:02 am
LGTM = Looks Good To Me?
My team has spent time working out if anything we do in a code review could be automated. Linting and formatting to an agreed, enforced set of rules being a case in point.
As someone who does a lot of reviews I guide my team towards certain practices.
Keep a PR (pull request) to a tightly defined piece of work. There's nothing worse than being asked to review changes across 30+ files of convoluted code. There is a feedback loop here to the way we define work. Defining work as small deliverables makes it far easier to have small focussed PRs.
If the nature of the change really does require a lot of files then keep your individual commits small and tightly focussed. That way I can review the code on a commit by commit basis.
In Github you can label your commits. Our labels are:-
These can be in almost any combination though having a PR that is only "Documentation" or "Good 1st issue" makes it easier for people new to PRs to get into the swing of things.
February 28, 2024 at 10:34 am
The decision on whether or not to pre-approve when I've suggested changes depends on a few things for me.
February 28, 2024 at 11:44 am
Maybe as important as code reviews could be what I would call code-flogging. Needs to be tested with a million rows and by testers intentionally trying to break it. My little home machine has tables of over 1.3 million rows and my queries do pretty good processing. Then I throw in some bad data to test error handling.
Rick
Disaster Recovery = Backup ( Backup ( Your Backup ) )
February 28, 2024 at 4:45 pm
Code reviews is another important topic, which I have no experience at. I'm in the unfortunate position of having never worked anywhere that did code reviews. I've wanted to do code reviews for years but am also kind of afraid of them. I'm concerned that someone, including myself, might get rude by asking what idiot wrote this piece of code. I'd like to be on a team of people where someone has experience at doing code reviews.
However, there's a very good chance that we'll be doing code reviews. Where I currently work there have been developers who are highly opinionated about how they write code and will NOT, under any circumstances, change. For example, about 5 years ago I had to fix a problem in a coworkers code, because he was out sick or on vacation (I don't remember which). He used the KeyValuePair struct in C#, but because he wanted to iterate through the collection using his own methodology, he essentially used that struct as a ValueKeyPair. I spent 4 hours trying to make it work as the documentation said it should work, until I realized my coworker did what he wanted to do and didn't document why he wasn't following the Microsoft documentation. Because he has the backing of our supervisor and HR wouldn't do anything about his bad behavior, he could do whatever he wants and no one will correct him.
But now we're moving to GitHub. GitHub has branch permissions (something that TFS never had), so I'm hoping that behavior that basically says, "It's my way or else, no matter how incompliant it is with standards!!" will be stopped. We'll see.
Kindest Regards, Rod Connect with me on LinkedIn.
February 28, 2024 at 5:33 pm
The decision on whether or not to pre-approve when I've suggested changes depends on a few things for me.
- How much do I trust that developer?
- How urgent is the change vs the severity of my suggested amendments? I won't usually hold something urgent back because the formatting/layout is non-standard.
- Has the developer had exactly the same amendments suggested in a previous review? If I've waved a previous change through because it was urgent, I expect the next change to conform with standards however urgent it is.
trust is definitely earned. I find myself applying different standards to different people over time.
Not sure that's the best policy
February 28, 2024 at 7:49 pm
I would say that the people at DirecTv should have some better code review. From what I read yesterday they were blaming an outage in my area on some coding issues that caused satellites to not be pointed correctly. Things are back to normal today.
I used to try and force code to be formatted a certain way. After awhile of this I was told to drop it since things worked and needed to get in prod ASAP.
When I have to update any code that was not formatted to the standards we were trying to set, I format it. It helps me read it and understand what is going on.
-------------------------------------------------------------
we travel not to escape life but for life not to escape us
Don't fear failure, fear regret.
February 28, 2024 at 7:53 pm
I would say that the people at DirecTv should have some better code review. From what I read yesterday they were blaming an outage in my area on some coding issues that caused satellites to not be pointed incorrectly. Things are back to normal today.
I used to try and force code to be formatted a certain way. After awhile of this I was told to drop it since things worked and needed to get in prod ASAP.
When I have to update any code that was not formatted to the standards we were trying to set, I format it. It helps me read it and understand what is going on.
git hooks can help with CLI formatters for this very reason.
February 29, 2024 at 3:43 am
Git hooks, huh? I've not heard of that. Sounds like something I'm going to learn. Thank you!
Rod
February 29, 2024 at 9:47 am
If you put this in a GitHub repository .github\workflows\check_labels.ymlfile then you can make running this a mandatory check in the "branch protection" option in your repo settings.
---
name: Label Checks
on:
pull_request:
types:
- opened
- labeled
- unlabeled
- synchronize
- edited
- reopened
branches:
- main # only trigger when PR into main
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
defaults:
run:
shell: bash
jobs:
checklabels:
name: Check Pull Request Labels
runs-on: ubuntu-latest
steps:
- name: Verify Labels
uses: jesusvasquez333/verify-pr-label-action@v1.4.0
with:
github-token: ${{ env.GITHUB_TOKEN }}
valid-labels: 'bug, dependencies, documentation, enhancement, security'
disable-reviews: true
pull-request-number: ${{ github.event.pull_request.number }}
enable_auto_merge:
name: Enable Auto Merge on PR
runs-on: ubuntu-latest
needs:
- checklabels
steps:
- name: Checkout Code
uses: actions/checkout@v3.6.0
with:
token: ${{ env.GITHUB_TOKEN }}
ref: ${{ github.event.pull_request.head.sha }}
- name: Enable auto-merge
run: |
gh pr merge --auto --merge "${PR_URL}"
env:
PR_URL: ${{github.event.pull_request.html_url}}
It insists that any PR must be labeled with at least one of
The 2nd job enables auto-merging into the main branch provided all branch protection criteria are met, not just the label conditions specified in this Github workflow file.
In my organisation these mandatory checks are as follows:-
One of the Github repo settings allows us to insist that there must be at least one approving review.
We use Renovate and Dependabot to auto-upgrade our dependencies and have a separate workflow file that auto-approves submissions from these two bots. In short, there are two industry standard bots that are actively patching our software dependencies. As long as our tests pass these are auto-approved and deployed to production. This is why we insist on such high test coverage.
A change made by a team member is subject to a PR. Part of the review is checking that the tests cover the business and technical requirements". A dependency upgrade from the bots will trigger those tests. Passing those tests mean that the upgrade has not changed the business and technical requirements expressed by those checks and is therefore safe to push to production.
It's all about making sure your mechanical testing regime is fit for purpose.
February 29, 2024 at 3:16 pm
Dang, @david.Poole, that is an awesome post with embedded YAML! I'm definitely going to keep a link to that post.
I do have a question though. In the jobs section, the "Check Pull Request Labels" has a runs-on: ubuntu-latest and then immediately afterwards in "Enable Auto Merge on PR" you have another runs-on: ubuntu-latest. I've not seen that done before. I always include a runs-on, but only once in my YAML. Why have you done that twice?
Kindest Regards, Rod Connect with me on LinkedIn.
February 29, 2024 at 6:05 pm
I do have a question though. In the
jobssection, the "Check Pull Request Labels" has aruns-on: ubuntu-latestand then immediately afterwards in "Enable Auto Merge on PR" you have anotherruns-on: ubuntu-latest. I've not seen that done before. I always include aruns-on, but only once in my YAML. Why have you done that twice?
I'm still learning about GitHub workflows. I keep meaning to do the official Github courses on them. I'm not 100% sure about the scope of certain settings.
I spent a lot of time getting a workflow to run SQLFluff on only the SQL files in a PR rather than the 300 in the repository. I suspect my solution is in the "works but could be done better" category. I did the best I could with the information I had available to me at the time.
Viewing 14 posts - 1 through 13 (of 13 total)
You must be logged in to reply to this topic. Login to reply