Group code reviews
Posted: 2023-01-29 16:03:38 by Alasdair Keyes
Whist working with a customer of mine, I was introduced to a way of code review that I hadn't seen before. I'm not sure if it's particularly unique to them, but in my experience it's fairly rare... group code review.
In all previous companies I've worked at/with, a pull request (PR) is created and has some key devs added for Code Review. For a speedy review a link to the PR was often thrown into the Dev Slack channel with a little prodding or begging to get the task done.
Now, most people in dev teams are busy; or at least if they're not too busy, they find almost anything more interesting than reviewing a PR. It can be tedious and if it's touching code that you're not familiar with, you can often end up spending some time trying to work out both what the code was doing and what the change is supposed to be doing. In the end it was often team leaders who ended up picking up the slack and performing the reviews to get code out the door. This puts an excessive burden on them when it could be shared more equally.
The process
This particular customer had group code reviews once in the morning after stand-up and then an optional code review mid afternoon if required. My first thought was that this would cause undue disruption to work flow but over time I saw immense benefit in this approach and very little drawback.
The developer that wrote the code would create a PR as normal. At the review, they would share their screen on a famous git-hosting provider. Usually they would give a sentence about the ticket and what they were trying to achieve. Due to stand-ups and backlog planning, everyone usually had a fairly good idea on this already. The dev would proceed to go through the changes on their branch, giving a run down of the code as they went. Questions or points could be made by anyone and a discussion could be had amongst the group. If new tests were made, they would also be run through briefly so that other devs could see what was being tested, whether it was suitable/adequate and any edge cases that might fall through the cracks and need to be added. (It also required that the tests passed!)
Any changes that were required and agreed upon by the team were added to the PR and the dev would go back and implement the changes.
If the changes needed were minor, they were often approved by a senior dev and scheduled for release. Larger changes would have to have a new PR created and go back into the code review process. The coder would only have to go through the changes from the last code review, not the whole shebang.
The benefits...
- Ensures domain experts see the code - The developer may not have much experience at the code being changed or a full understanding of the system. Other members who have this knowledge can ensure that the changes would fit in with the design and not break anything.
- Ensures non-domain experts see new code - This is fantastic, especially for new members. Seeing the changes and how they integrate with the existing code allows people to get a feel for how the system works without having to delve into the code personally. They can easily pick up any design patterns that may be in use and understand the code/class hierarchy and layout. Over time they will gain a general awareness of the code even though they've not touched it directly. A kind of learning by osmosis.
- Team leaders can assess team members - This provides rolling assessments of the knowledge and skill of their team without the need for any separate review process. Team leaders can make notes on progress and growth for one-on-one meetings and also use the knowledge to help assign future tickets to team members.
- Code practices can be debated and decided - It's a single time/place where code practices can be hammered out. Getting consensus on the way things should be done can be difficult, especially in any group larger than 4 or 5. Devs can explain why they've chosen to tackle a problem in a specific way. Others can dispute it and code-practices and styles can be decided on for the future. Everyone (or almost everyone) is there and can have their say and a consensus can be reached.
- A chance to explain - When reviewing code, you often just see the end product and may have ideas on why it's wrong or how it could be improved. Maybe you feel the wrong design pattern was used? When these issues are raised, a group setting gives devs an arena to explain why changes were made the way they were. They can explain thought processes and previous attempts at a solution and why they were discarded. Just looking at a code review on the screen doesn't explain the knowledge and thought that went into the code and why X process was adopted instead of Y to solve the problem.
- Reduce time-to-release - This method reduces the time that PRs are sat around waiting for review by forcing them to be reviewed. Code reviews are dull. Unless you're the sadistic type that likes to just tear into other people's code, you can always think of better ways to spend your time. This way devs are not having to hassle people to get code reviews to release to production and wasting time whilst waiting.
- More equality in the burden of code review - That developer who always ends up reviewing code because no-one else does finally gets a break. No more "You've been added to review code" emails and the responsibility gets shared amongst the team.
Things you think are bad... but they're not as bad as you think...
- Minimal break in flow - No one wants to be disturbed and have to stop coding when they're making progress. Having the morning review directly after the stand-up means that no-one has to break their concentration again as they were already in the stand-up.
- Minimal break in flow... again - The afternoon review is optional and based on whether there are reviews needed. Usually the message
Code reviews?was thrown into Slack and others would only reply if they had one. If there are only small reviews that aren't too important and not holding anyone back, they could be done at the next morning review. Everyone can then continue with their work uninterrupted. Only break flow if there is a need. - Time loss - You might do some quick maths and work out that 10 developers spending 15 minutes is 2.5 hours lost each day. That's true, but it's only 3% of dev time each day ()assuming an 8 hour day) and the benefits listed above are well worth it. And lets face it, we probably all spend more than 15 minutes a day getting coffee, smoking/vaping, eating snacks... it's not a huge loss.
- Introduction to public speaking - Very few people like standing up in front of peers to talk, even less so when you feel you're having to explain yourself and your work; but it's not a bad thing to get used to. Explaining things and defending your position are good life skills in general. After a few reviews, you realise it's not all that bad and it builds team cohesion, trust and over-all confidence in your code and abilities.
- Introduction to tact and useful criticism - The flip side of the above point is that it requires the other developers to show tact and diplomacy when reviewing code... especially with junior or newer members. Also remember that often devs won't just have knocked out a big commit of code without any input from the team. The implementation may have already been discussed between the team so there's not going to be anything too damning to criticise.
- People playing with laptops - A group code review like this often causes laptops to be in the code review. We all know that if there's a computer in front of you, there's a temptation to use it. Some ground rules can be set about closing machines when not specifically used for the review.
So that's my review on group code-review. If you're looking for possible improvements to your process I strongly advise you give it a shot. Do it for a month or so and see how you get on. You can always tweak the process based on your needs and fall back to your previous ways if you don't like it.
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
Adding custom Firefox config with Ansible
Posted: 2023-01-26 21:05:42 by Alasdair Keyes
I've been writing Ansible playbooks to build my laptop and I came across a problem with applying my custom Firefox user.js config file to a new build.
Firefox won't create config/profile directory until it's started for the first time. During this first start a random profile directory is created in which the user.js file needs to be placed e.g /.mozilla/firefox/wxyz1234.default-release/.
As such you can't just install user.js with a basic file copy.
To counter this I wrote the following play so that if there was no Firefox profile folder, Ansible will start Firefox, allow it to create the profile folder and then kill it. It will then search for the newly created profile folder and install the file.
# Install Firefox
- name: Install Firefox
become: true
package:
name:
- firefox
state: present
# Check if a profile folder exists
- name: Check Firefox config folder
find:
paths: "{{ ansible_user_dir }}/.mozilla/firefox"
patterns: '^.*\.default-release'
use_regex: true
file_type: directory
register: firefox_config_folder_search
# If profile folder doesn't exist, start Firefox
- name: Starting Firefox
shell:
cmd: "firefox &"
changed_when: false
when: firefox_config_folder_search.matched == 0
- name: Waiting for Firefox to start
command: sleep 10s
changed_when: false
when: firefox_config_folder_search.matched == 0
# Kill Firefox
- name: Killing Firefox
shell:
cmd: kill $(pidof firefox)
changed_when: false
when: firefox_config_folder_search.matched == 0
# Search for the newly created profile directory
- name: Check Firefox config folder again
find:
paths: "{{ ansible_user_dir }}/.mozilla/firefox"
patterns: '^.*\.default-release'
use_regex: true
file_type: directory
register: firefox_config_folder_search
# Set a fact with the profile directory path
- name: Set firefox folder name as fact
set_fact:
firefox_config_folder_path: "{{ firefox_config_folder_search.files[0].path }}"
# Add in the custom config
- name: Add in Firefox config
copy:
src: files/firefox/user.js
dest: "{{ firefox_config_folder_path }}/user.js"
owner: "{{ ansible_user_id }}"
group: "{{ ansible_user_id }}"
mode: 0644
when: firefox_config_folder_path != 'false'
The play is idempotent so you can run it as many times as you want and it will continue to give you a nice green response.
I only ever use a single Firefox profile so I don't need to ensure different configs, but it could be extended to take this into account if you needed.
I later found some other software I use has this same config issue so I extracted the process start/kill tasks into a separate file
- name: Starting process {{ item }}
shell:
cmd: "{{ item }} &"
changed_when: false
- name: Waiting for proces {{ item }} to start
command: sleep 10s
changed_when: false
- name: Killing process {{ item }}
shell:
cmd: kill $(pidof {{ item }})
changed_when: false
Which can then be called with the following in your playbook
- name: Start/Kill myprogram to generate config
include_tasks: start-kill-process.yml
loop:
- myprogram
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
The RAID
Posted: 2023-01-08 21:15:00 by Alasdair Keyes
After my failed drive on New Year's day, I ordered a new disk and rebuilt the array. Thankfully due to monthly checking by the OS, all the data on the three remaining drives was readable and the array is complete again.
I put the failed disk into another machine and ran the following badblocks command on it.
badblocks -o sdb_badblocks.txt -b 4096 -w -s /dev/sdb
I used the destructive test as the data was not needed now that the array was back to full strength. Incidentally, using a block size of 4096 over the default 1024 seemed to provide about a 2x-3x speed increase.
Even with that, the 2TB disk took just over 33 hours for a full write pass and a confirmation read pass.
At the end of it, a full write and read pass were managed with no errors reported. This is frustrating as mdadm had obviously detected a read error to reject the disk - this was logged in syslog.
I thought that maybe the bad sectors had been remapped by the firmware during the badblocks test, but checking the SMART stats again I saw that no errors are reported and also no re-allocation had been logged (ID# 5 below).
SMART Attributes Data Structure revision number: 16
Vendor Specific SMART Attributes with Thresholds:
ID# ATTRIBUTE_NAME FLAG VALUE WORST THRESH TYPE UPDATED WHEN_FAILED RAW_VALUE
1 Raw_Read_Error_Rate 0x000b 100 100 016 Pre-fail Always - 0
2 Throughput_Performance 0x0005 134 134 054 Pre-fail Offline - 103
3 Spin_Up_Time 0x0007 168 168 024 Pre-fail Always - 342 (Average 311)
4 Start_Stop_Count 0x0012 100 100 000 Old_age Always - 75
5 Reallocated_Sector_Ct 0x0033 100 100 005 Pre-fail Always - 0
7 Seek_Error_Rate 0x000b 100 100 067 Pre-fail Always - 0
8 Seek_Time_Performance 0x0005 146 146 020 Pre-fail Offline - 29
9 Power_On_Hours 0x0012 086 086 000 Old_age Always - 99078
10 Spin_Retry_Count 0x0013 100 100 060 Pre-fail Always - 0
12 Power_Cycle_Count 0x0032 100 100 000 Old_age Always - 75
192 Power-Off_Retract_Count 0x0032 100 100 000 Old_age Always - 989
193 Load_Cycle_Count 0x0012 100 100 000 Old_age Always - 989
194 Temperature_Celsius 0x0002 200 200 000 Old_age Always - 30 (Min/Max 16/44)
196 Reallocated_Event_Count 0x0032 100 100 000 Old_age Always - 0
197 Current_Pending_Sector 0x0022 100 100 000 Old_age Always - 0
198 Offline_Uncorrectable 0x0008 100 100 000 Old_age Offline - 0
199 UDMA_CRC_Error_Count 0x000a 200 200 000 Old_age Always - 0
So I'm not sure why the error occurred. Maybe the controller is bad, the cable is dodgy or my server got hit by some stray cosmic rays somewhere and caused some kind of CRC error (yes, it can and does happen). The server has ECC memory so bitflips in the RAM should have been detected had they occurred.
Interestingly, this is the first failed disk I've had within a Linux MDADM array in over 20 years of running servers (I've had plenty of failed disks in Dell PERC controllers and whatever controllers Supermicro jam into their servers!). All previous arrays have been torn down before a disk failed.
As such this was also the first time I've had to rebuild an array. This particular RAID was running for over 11 years before this disk failed. For those interested, I followed this post by Redhat about the steps to take https://www.redhat.com/sysadmin/raid-drive-mdadm.
Should something similar happen again, I think I would run badblocks in non-destructive mode on the disk in situ, then if it passed push it back into the array for it to be rebuilt before I looked at buying a new disk.
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
Best Albums of 2022
Posted: 2023-01-02 14:03:49 by Alasdair Keyes
My most enjoyed albums of 2022...
- Tears for Fears - The Tipping Point
- Scorpions - Rock Believer
- Red Hot Chili Peppers - Return of the Dream Canteen
- KMFDM - Hyëna
- Crystal Method - The Trip Out
A special mention to Röyksopp's trilogy of albums Profound Mysteries I/II/III and a special note of disappointment on the new albums from Megadeth and Rammstein, both had a couple of good tracks, but nowhere near as good as I'd expect.
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
Happy New Year.... It would be a shame if that drive failed.
Posted: 2023-01-01 10:49:52 by Alasdair Keyes
A delightful New Year's Day gift.
A Fail event had been detected on md device /dev/md/0.
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
Javascript Asyncronous coding in
Posted: 2022-08-10 18:49:54 by Alasdair Keyes
Being a (primarily) back-end developer, my use of Javascript has been fairly limited. Mostly using jQuery and some basic raw Javascript in the olden-days for frontend validation, basic animation and alerts etc.
I've been writing a project in Quasar https://quasar.dev/ and delving further into the mysteries of async programming.
I was struggling to understand some documentation I found online. After some digging, it turns out that through various versions of ECMAScript, the concept of asynchronous programming has evolved and changed, giving you multiple ways of doing things with callbacks, promises and async/await.
I found this video on Youtube https://www.youtube.com/watch?v=PoRJizFvM7s which although only 24 minutes long gives a fantastic introduction to how each iteration of callbacks/promises/async/await has been developed, how they work and how to code against them.
It gives and intro with examples and that was all I need to get it straight in my head and make some real progress. It's a must-watch for anyone starting to get involved with Javascript other than simple libraries like jQuery.
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
Setting remote IP on Laravel controller tests
Posted: 2022-08-07 12:45:15 by Alasdair Keyes
When building a Laravel website you might want to create allow/block lists based on user's IP or from GeoIp information. This is easy enough using geoip2/geoip2 (https://packagist.org/packages/geoip2/geoip2) but how do you test your code is working correctly with specific IP addresses when writing your functional/integration tests?
At the beginning of a test that requires a custom IP you can add $this->serverVariables = ['REMOTE_ADDR' => '1.2.3.4']; and this will be what your controller sees in the IlluminateHttpRequest object.
public funtion testGeoIpFunctionality(): void
{
$this->serverVariables = ['REMOTE_ADDR' => '1.2.3.4'];
$response = $this->get('/website/endpoint');
$response->assertStatus(200);
...
// other assertions
...
}
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
"They" are watching... even quicker than before
Posted: 2022-05-06 20:45:08 by Alasdair Keyes
I wrote a blog post in 2019 about the website of a newly registered domain getting visited by a bot within 5 hours of the website coming online. You can read the article here - Security first, "they" are watching.
In short, I had surmised that the Certificate Transparency logs were being monitored to discover new sites so they could be scanned for vulnerabilities before an admin had a chance to harden the website.
I read an article today (https://portswigger.net/daily-swig/wordpress-sites-getting-hacked-within-seconds-of-tls-certificates-being-issued) which looks as if this premonition has come to pass. Wordpress websites are apparently getting hacked 'within seconds' of the TLS certificates being issue.
It looks like the logs are being tailed and visited much quicker than before... from 5 hours 3 years ago to <1 minute today.
I've steered clear of Wordpress for years now and often advise my clients to do the same. Although the usability and extensibility of Wordpress is fantastic, the scope for vulnerabilities in both plugins and the core code is too great to rely on. If you do run it, assess if you really need it for a public facing site and if you don't, add IP or Basic Authentication restrictions to your webserver config to restrict access to only those who need it.
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
Reducing dependencies and expanding Laravel Blade
Posted: 2022-05-05 08:05:06 by Alasdair Keyes
I've recently spent a couple of days moving my site to the latest version of Laravel. There were some problems upgrading through such a large number of major versions which I'll likely do a blog post about later.
Whilst I was re-adding my Composer dependencies I was looking at what I really needed. My blog posts are written in markdown and stored in a the database, the Laravel template engine, Blade, converts them from Markdown to HTML. For this task I was using the parsedown/laravel plugin (https://packagist.org/packages/parsedown/laravel)which uses erusev/parsedown (https://packagist.org/packages/erusev/parsedown)underneath to do the actual markdown processing.
I try to minimise dependencies used for two reasons,
- Reduced complexity in the codebase
- Reduced attack vectors either from attacks directly against my site or supply chain attacks through PHP's Composer system.
Whilst browsing through the Laravel Docs I noticed that they have an inbuilt Str::markdown (https://laravel.com/docs/9.x/helpers#method-str-markdown) helper which might allow me to do the same thing. Under the hood it uses Commonmark from the PHP League (https://commonmark.thephpleague.com/)
I used a couple of custom options on Parsedown, which I needed to be sure worked with the Laravel version.
$parseDown = Parsedown::instance();
$parseDown->setUrlsLinked(false);
$parseDown->setMarkupEscaped(false);
setUrlsLinked: false means that URLs aren't automatically converted into a href links and setMarkupEscaped: false means that I can include HTML markup in my blog posts if I desire.
After reading through the Common mark docs I the relative options were allow_unsafe_links: true and html_input: allow flag and I'd be set. Although these are defaults for Commonmark, I want to explicitly declare them in case defaults change in future.
I only used markdown in my templates and Parsedown automatically adds a blade directive of @parsedown("# Markdown Title") which I made use of. My first task was to create a Blade directive to process markdown in my templates, I decided on the name processMarkdown()
I created app/Providers/CustomBladeFunctionProvider.
<?php
declare(strict_types=1);
namespace App\Providers;
use Illuminate\Support\Facades\Blade;
use Illuminate\Support\ServiceProvider;
use Illuminate\Support\Str;
class CustomBladeFunctionProvider extends ServiceProvider
{
/**
* Bootstrap the application services.
*
* @return void
*/
public function boot(): void
{
// Provide @processMarkdown
Blade::directive('processMarkdown', function ($parameter) {
return "<?= rtrim(Str::markdown($parameter, [ 'allow_unsafe_links' => true, 'html_input' => 'allow' ])); ?>";
});
}
}
Then added this to my providers in config/app.php.
...
'providers' => [
...
App\Providers\CustomBladeFunctionProvider::class,
...
],
...
Then all I needed to do was update my templates from
@parsedown($blogPost->body)
@processMarkdown($blogPost->body)
And then remove the parsedown/laravel dependency to slim down my codebase.
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
Language Transfer for learning a new language
Posted: 2022-04-13 12:21:46 by Alasdair Keyes
I've recently tried to get back into learning German and I have started using the Language Transfer website.
Language Transfer is run by one chap, Mihalis who teaches a range of languages, French, Italian, German, Spanish, Greek, Turkish, Arabic and even Swahili to English speakers. His concept for learning languages is to understand what shared parts of English are similar (or transferred) across into the language you are learning and use that as a base to get a fast grounding.
I've been using Duolingo on and off for a while, but often become frustrated in it's lack of explanation as to why certain aspects of the language are the way they are. Language Transfer really adds to it by teaching common rules as to how to construct sentences in the given language, how verbs congugate, nouns pluralise, etc.
If you're learning any languages, I highly recommend checking the site to see if he is teaching your language and I think you'll find it a great help.
https://www.languagetransfer.org/
If you found this useful, please feel free to donate via bitcoin to 1NT2ErDzLDBPB8CDLk6j1qUdT6FmxkMmNz
© Alasdair Keyes
IT Consultancy Services
I'm now available for IT consultancy and software development services - Cloudee LTD.
Happy user of Digital Ocean (Affiliate link)
Version:master-53c82addfa
