Add Tag manager script by yaswant · Pull Request #185 · MetOffice/SimSys_Scripts

yaswant · 2026-02-09T21:47:57Z

PR Summary

Sci/Tech Reviewer: @r-sharp
Code Reviewer: @t00sa

Script to manage (add, delete, list) tags in a repository.

Code Quality Checklist

I have performed a self-review of my own code
My code follows the project's style guidelines
Comments have been included that aid understanding and enhance the readability of the code
My changes generate no new warnings
All automated checks in the CI pipeline have completed successfully

Testing

This change has been tested appropriately (please describe)

Security Considerations

I have reviewed my changes for potential security issues
Sensitive data is properly handled (if applicable)
Authentication and authorisation are properly implemented (if applicable)

AI Assistance and Attribution

Some of the content of this change has been produced with the assistance of Generative AI tool name (e.g., Met Office Github Copilot Enterprise, Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the Simulation Systems AI policy (including attribution labels)

Sci/Tech Review

I understand this area of code and the changes being added
The proposed changes correspond to the pull request description
Documentation is sufficient (do documentation papers need updating)
Sufficient testing has been completed

(Please alert the code reviewer via a tag when you have approved the SR)

Code Review

All dependencies have been resolved
Related Issues have been properly linked and addressed
Code quality standards have been met
Tests are adequate and have passed
Security considerations have been addressed
Performance impact is acceptable

r-sharp

Given the following comment :

Requires:

GitHub CLI (gh): https://cli.github.com/

Git command-line tool: https://git-scm.com/

A similar reference should be made to 'jq' as a JSON specific 'sed' like tool is hardly commonplace.

Also, given the number of subroutines and relieance on required non standard 'libraries', I do have to question if bash was the tool the job. I can't help but feel something more adept at variable handling and argument passing would have been a better tool and would have been easier for subsequent maintainers to read and follow.

Taking the first argument as a comment and then essentially executing the remaining arguments to test for a non-zero return code doesn't strike me as the healthiest way to go about things. This has the feel of something hacked together to use at a specific time, and then throw away, rather than something to be retained for future use which would mean it's perhaps best not added to the repository.

r-sharp · 2026-02-11T13:17:47Z

sbin/tagman

+
+set -euo pipefail
+# Colour codes for output
+GRN='\033[0;32m'


Suggested change

GRN='\033[0;32m'

GREEN='\033[0;32m'

Please use full words in variable names for future clarity.

Thank you for the feedback. However, NC (No Color) is an extremely well-established convention in bash scripting for ANSI colour codes, alongside RED, GRN (Green), YLW (Yellow), etc. These abbreviations are widely recognised in the shell scripting community. Using full words like NO_COLOR or RESET_COLOR_CODE would make the code unnecessarily verbose without improving clarity for anyone familiar with bash scripting conventions.

I'm afraid I disagree.
The fact that I didn't know what NC was indicates it's not a sufficiently7 well known convention.

Variable names should be full, not abbreviated, words wherever possible for clarity.

r-sharp · 2026-02-11T13:18:17Z

sbin/tagman

+# Colour codes for output
+GRN='\033[0;32m'
+RED='\033[0;31m'
+YLW='\033[0;33m'


Suggested change

YLW='\033[0;33m'

YELLOW='\033[0;33m'

Please use full words in variable names for future clarity.

r-sharp · 2026-02-11T13:19:18Z

sbin/tagman

+GRN='\033[0;32m'
+RED='\033[0;31m'
+YLW='\033[0;33m'
+NC='\033[0m'


What is 'NC' ?
not remotely clear, Plaese use full words in variable names.

r-sharp · 2026-02-11T13:22:38Z

sbin/tagman

+Usage:
+  $(basename "$0") add <tag_name> <commit_ref> [options]
+  $(basename "$0") delete|del <tag_name> [options]
+  $(basename "$0") list|ls [options]
+
+Actions:
+  add       Create and push a new tag
+  delete    Delete a tag from the repository (alias: del)
+  list      List all tags in the repository (alias: ls)
+
+Arguments:
+  <tag_name>     Name of the tag to create or delete
+  <commit_ref>   Commit SHA, tag name, release name, or branch name
+
+Options:
+  --repo, -R REPO    Repository in format owner/repo (default: $DEFAULT_REPO)
+  --message MSG      Tag annotation message (for add action)
+  --dry-run, -n      Show what would be done without making changes
+
+Examples:
+  # Create tag from commit SHA
+  $(basename "$0") add v1.0.0 abc123def --repo MetOffice/git_playground
+
+  # Create tag from existing tag
+  $(basename "$0") add Test vn1.5 --repo MetOffice/git_playground
+
+  # Create tag from release
+  $(basename "$0") add Autumn2025 vn14.0 --repo MetOffice/um
+
+  # Create tag from branch
+  $(basename "$0") add Spring2026 main --repo MetOffice/SimSys_Scripts


Too Long.
I can't even get the whole 'Usage' block on a single screen in order to highlight it for a comment.
43 lines is going to be more than most terminal heights.
Consider significantly less examples, which seem to be overkill

I understand the concern about screen real estate, but I respectfully disagree that this is excessive. Still I have a feeling that you can convince me to shorten this ;)

The usage function serves as both immediate help and documentation for users who may not have access to or won't read separate documentation. It provides:

Clear syntax for all three actions (add/delete/list)

Available options and their formats

Five practical examples covering different use cases

Important notes about behaviour

⚠️ This script performs potentially destructive Git operations (creating/deleting tags in production repositories). Comprehensive examples aren't "overkill" - they help prevent user errors that could have significant consequences. Each example demonstrates a distinct use case:

Tag from commit SHA (most common)

Tag from existing tag

Tag from release (cross-repo workflow)

Tag from branch

Delete operation

Users can scroll or pipe to less if needed: tagman | less
Terminal heights vary, but scrolling is expected for help text
If there's a specific concern about this blocking adoption or causing issues, I'm open to discussing it, but cutting back examples would make the tool harder to use correctly, especially for destructive operations.

I don't feel all the examples are neccessary as there is insufficient difference between what needs to be written (as a command)

The first 3 examples are all the same. Purely the syntax of the commit being supplied ha=s changed. There is no fundemental difference between the three examples.

In FCM we didn't feel the need to provide examples using revisions & keywords & tags as all three were interchangable.

"Terminal heights vary, but scrolling is expected for help text" it most certainly isn't.

Even this response is TL;DR

r-sharp · 2026-02-11T13:25:24Z

sbin/tagman

+}
+
+cleanup() {
+  [[ -n "${WORK_TMP:-}" ]] && rm -rf "$WORK_TMP"


Suggested change

[[ -n "${WORK_TMP:-}" ]] && rm -rf "$WORK_TMP"

if test -n "${WORK_TMP:-}"; then

rm -rf "$WORK_TMP"

fi

Please use full, clear code, not clever contractions. This isn't Java.

I appreciate your attention to code clarity. The [[ ]] construct is actually the recommended modern bash syntax over test or [ ]. It's more robust, handles edge cases better (like empty strings), supports pattern matching, and is explicitly recommended in the Google Shell Style Guide and other authoritative sources. This isn't a "clever contraction" but rather best practice for bash scripting.

The [[]] isn't the contraction I'm refferring to "&&" is
Please use a proper if then statement.

r-sharp · 2026-02-11T16:28:32Z

sbin/tagman

+
+  case "$ACTION" in
+    add)
+      [[ $# -lt 2 ]] && usage


Suggested change

[[ $# -lt 2 ]] && usage

if test $# -lt 2 ; then

usage

fi

as above - I don't see why not.

r-sharp · 2026-02-11T16:29:01Z

sbin/tagman

+      shift 2
+      ;;
+    del|delete)
+      [[ $# -lt 1 ]] && usage


Suggested change

[[ $# -lt 1 ]] && usage

if test $# -lt 1 ; then

usage

fi

r-sharp · 2026-02-11T16:29:50Z

sbin/tagman

+      # No arguments required
+      ;;
+    *)
+      echo -e "${RED}Unknown action: $ACTION${NC}"


Suggested change

echo -e "${RED}Unknown action: $ACTION${NC}"

echo -e "${RED}Unknown action: $ACTION${NORMAL_COUNTENANCE}"

Suggested change cannot be applied.

but a change is still required.

r-sharp · 2026-02-11T16:31:11Z

sbin/tagman

+      --message) MESSAGE="$2"; shift 2 ;;
+      -n|--dry-run) DRY_RUN=true; shift ;;
+      *)
+        echo -e "${RED}Unknown option: $1${NC}"


Suggested change

echo -e "${RED}Unknown option: $1${NC}"

echo -e "${RED}Unknown option: $1${NASTY_CARROT}"

Suggested change cannot be applied.

r-sharp · 2026-02-11T16:32:20Z

sbin/tagman

+    add) add_tag ;;
+    del|delete) delete_tag ;;
+    ls|list) list_tags ;;
+     *) echo -e "${RED}Invalid action: $ACTION${NC}" ;;


Suggested change

*) echo -e "${RED}Invalid action: $ACTION${NC}" ;;

*) echo -e "${RED}Invalid action: $ACTION${NOTABLE_CAULIFLOWER}" ;;

Suggested change cannot be applied.

yaswant · 2026-02-13T02:14:41Z

@r-sharp I appreciate you taking the time to review this thoroughly. Thanks for the suggestion re jq - I'll update the comment section to include it. However, I respectfully disagree with this assessment. I've replied to your individual comments/suggested changes as well.

Regarding tool choice:
Bash is the appropriate tool for this task. The script's purpose is Git tag management - fundamentally a command-line operation that requires executing git/gh commands, which bash excels at. The "non-standard libraries" you mention are:

GitHub CLI (gh) - the official GitHub tool
jq - industry-standard JSON processor
Git itself

These are standard tools in our development environment and are explicitly documented in the requirements. Rewriting this in Python or another language would add unnecessary complexity, dependencies, and overhead for what is essentially orchestrating CLI commands.

Regarding maintainability:
The code is well-structured with descriptive function names, clear separation of concerns, and extensive comments. Anyone familiar with bash (a core skill for system administrators and DevOps engineers) can maintain this.

I welcome discussion about genuine improvements, but the suggestion that this is throwaway code or shouldn't be in the repository doesn't reflect the actual code quality or appropriateness of the implementation.

r-sharp

Bash may well be the appropriate tool for executing shell commands, it is not a particualrly good tool for more involved programming such as concoluted logic, subroutines and operations with multiple paths throughthe operation.

""" The "non-standard libraries" you mention are:

GitHub CLI (gh) - the official GitHub tool
jq - industry-standard JSON processor
Git itself

"""
I only mentioned them, because you felt the need to. The fact that you felt the need to list them as prerequestits indicates that there is an acceptance on your part that they may not be present.
My feeling was that if you had to ensure the presence of those libraries for use with bash, was no different from having to ensure similar librararies to perform the same functions where equally available in a python script.

I don't agree that this would be more complex in python, it would be broadly similar but wouldn't be quite so convoluted.

"""Regarding maintainability:
The code is well-structured with descriptive function names, clear separation of concerns, and extensive comments."""
I disagree, or I wouldn't have added the comments I added.
All the comments I made are where the code was not clear and where I had pause reading the logic to think and backtrack to establish what was goping on.

If contractions and abbreviations are so easy to follow, then why are natural langue models/ML and AI becoming so popular ? Verbosity is not a bad thing if it improves the speed at which the information can be ingested and adsorbed without the need for additional processing.
Not having to translate what is being read into something with meaning internally saves time and effort.

"""Anyone familiar with bash (a core skill for system administrators and DevOps engineers) can maintain this."""
Then I guess you'd better sack me and emploty someone with those skills instead.

The purpose of a review is to provide feedback and appropriate requested changes.
If you're just going to overule the reviewer then we might as well jusst rubber stamp everything and move on.
I have made comments where I had to think about what you were trying to do rather than just reading a logical progression through a series of commands.

I will not accept YLW, GRN, NC, && and ||, They are not immediuately clear to the reader and would be improved by YELLOW, GREEN, , "if ; then" and " if !; then"

Add Tag manager script

b16f89c

github-actions bot assigned yaswant Feb 9, 2026

github-actions bot requested a review from james-bruten-mo February 9, 2026 21:48

yaswant added this to the Spring 2026 milestone Feb 9, 2026

yaswant added the enhancement New feature or request label Feb 9, 2026

github-actions bot requested a review from r-sharp February 10, 2026 09:37

yaswant removed the request for review from james-bruten-mo February 10, 2026 09:38

r-sharp requested changes Feb 13, 2026

View reviewed changes

yaswant requested review from r-sharp and t00sa February 13, 2026 02:23

yaswant and others added 7 commits February 13, 2026 02:26

Merge branch 'main' into tagman

d49a9fe

Add jq requirement and improve color code comments

e6c4c98

Merge branch 'tagman' of github.com:yaswant/SimSys_Scripts into tagman

101ebef

Update sbin/tagman

1fb5fc3

Refactor conditionals for clarity and consistency

56133c6

Merge branch 'tagman' of github.com:yaswant/SimSys_Scripts into tagman

60cb6ac

Reduce example calls in usage

b2d9710

r-sharp reviewed Feb 13, 2026

View reviewed changes

yaswant requested a review from r-sharp February 13, 2026 10:02

Update warnings in tagman script to emphasise responsibility

549b7a5

r-sharp requested changes Feb 13, 2026

View reviewed changes

yaswant requested a review from r-sharp February 13, 2026 11:19

	echo -e "${RED}Unknown action: $ACTION${NC}"
	echo -e "${RED}Unknown action: $ACTION${NORMAL_COUNTENANCE}"

	echo -e "${RED}Unknown option: $1${NC}"
	echo -e "${RED}Unknown option: $1${NASTY_CARROT}"

	*) echo -e "${RED}Invalid action: $ACTION${NC}" ;;
	*) echo -e "${RED}Invalid action: $ACTION${NOTABLE_CAULIFLOWER}" ;;

Conversation

yaswant commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Code Quality Checklist

Testing

Security Considerations

AI Assistance and Attribution

Sci/Tech Review

Code Review

Uh oh!

r-sharp left a comment

Choose a reason for hiding this comment

Requires:

GitHub CLI (gh): https://cli.github.com/

Git command-line tool: https://git-scm.com/

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yaswant commented Feb 9, 2026 •

edited

Loading

r-sharp left a comment •

edited

Loading