Skip to content

Conversation

@paigewilliams
Copy link
Contributor

Submitting as draft to accompany forthcoming proposal on versioning in TEK/ITK DB!

part of spike task

Displays git tag version number in admin UI

Screenshot 2025-11-14 at 9 43 52 AM

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements dynamic version retrieval from git tags to display in the admin UI, replacing the hardcoded version string "2.2.2" with the latest git tag. This supports the versioning strategy outlined in the associated spike task.

  • Replaces hardcoded VERSION string with dynamic git tag retrieval using subprocess
  • Updates ADMIN_SITE_HEADER format string to accommodate git tag format
  • Adds error handling for cases where git command fails

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

VERSION = "2.2.2"
try:
VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
except Exception:
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a bare except Exception is too broad and will catch all exceptions, including system-level exceptions like KeyboardInterrupt and SystemExit. Consider catching more specific exceptions like subprocess.CalledProcessError, FileNotFoundError, or OSError instead.

Example:

try:
    VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
except (subprocess.CalledProcessError, FileNotFoundError, OSError):
    VERSION = "unknown"
Suggested change
except Exception:
except (subprocess.CalledProcessError, FileNotFoundError, OSError):

Copilot uses AI. Check for mistakes.
try:
VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
except Exception:
VERSION = "unknown"
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This git command execution may fail in production environments where:

  1. The .git directory is not present (common in Docker containers or deployment packages)
  2. Git is not installed
  3. The repository has no tags yet

Consider adding a fallback mechanism or documenting the deployment requirements. For containerized deployments (as seen in your Dockerfile), you might want to:

  • Include the .git directory in the Docker build context, or
  • Set the VERSION via an environment variable during deployment, or
  • Write the version to a file during the build process

Example with environment variable fallback:

try:
    VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
except (subprocess.CalledProcessError, FileNotFoundError, OSError):
    VERSION = os.environ.get("APP_VERSION", "unknown")
Suggested change
VERSION = "unknown"
VERSION = os.environ.get("APP_VERSION", "unknown")

Copilot uses AI. Check for mistakes.
Comment on lines +234 to +238
try:
VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
except Exception:
VERSION = "unknown"

Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The subprocess call executes every time the settings module is imported, which can happen multiple times during the application lifecycle (e.g., management commands, worker processes, tests). Consider caching the result or using lazy evaluation to avoid repeated git command executions.

Example with lazy evaluation:

_VERSION = None

def get_version():
    global _VERSION
    if _VERSION is None:
        try:
            _VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
        except (subprocess.CalledProcessError, FileNotFoundError, OSError):
            _VERSION = os.environ.get("APP_VERSION", "unknown")
    return _VERSION

VERSION = get_version()

Alternatively, consider setting the version once during deployment (e.g., in an environment variable or a version file) rather than executing git at runtime.

Suggested change
try:
VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
except Exception:
VERSION = "unknown"
_VERSION = None
def get_version():
global _VERSION
if _VERSION is None:
try:
_VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
except (subprocess.CalledProcessError, FileNotFoundError, OSError, Exception):
_VERSION = "unknown"
return _VERSION
VERSION = get_version()

Copilot uses AI. Check for mistakes.

ADMIN_SITE_HEADER = os.environ.get(
"ADMIN_SITE_HEADER", default="ITK DB Admin v{}".format(VERSION)
"ADMIN_SITE_HEADER", default="ITK DB Admin {}".format(VERSION)
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The "v" prefix was removed from the admin header format string, likely assuming git tags include the "v" prefix (e.g., "v2.2.2"). However, this creates an inconsistency:

  • If tags have "v" prefix: displays "ITK DB Admin v2.2.2" ✓
  • If tags don't have "v": displays "ITK DB Admin 2.2.2" (different from old format)
  • If git fails: displays "ITK DB Admin unknown" (no "v")

Consider either:

  1. Stripping the "v" prefix from git output to maintain consistent formatting with "v{}"
  2. Documenting the expected tag format (e.g., tags should use "v" prefix)

Example for option 1:

VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip()
VERSION = VERSION.lstrip('v')  # Remove 'v' prefix if present
# Then use format string: "ITK DB Admin v{}".format(VERSION)

Copilot uses AI. Check for mistakes.
@paigewilliams
Copy link
Contributor Author

closing in favor of #218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants