Conversation
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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"| except Exception: | |
| except (subprocess.CalledProcessError, FileNotFoundError, OSError): |
| try: | ||
| VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip() | ||
| except Exception: | ||
| VERSION = "unknown" |
There was a problem hiding this comment.
This git command execution may fail in production environments where:
- The
.gitdirectory is not present (common in Docker containers or deployment packages) - Git is not installed
- 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
.gitdirectory 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")| VERSION = "unknown" | |
| VERSION = os.environ.get("APP_VERSION", "unknown") |
| try: | ||
| VERSION = subprocess.check_output(["git", "describe", "--tags", "--abbrev=0"]).decode("utf-8").strip() | ||
| except Exception: | ||
| VERSION = "unknown" | ||
|
|
There was a problem hiding this comment.
[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.
| 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() |
|
|
||
| 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) |
There was a problem hiding this comment.
[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:
- Stripping the "v" prefix from git output to maintain consistent formatting with "v{}"
- 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)|
closing in favor of #218 |
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