Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactored NuGet package versioning to use trigger-based updates instead of fetching latest versions, while removing automatic Docker image bumps from the CI workflow. The new approach selectively updates packages matching the trigger source and applies changes only when explicitly triggered. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR simplifies the service update automation by restricting NuGet package version bumping to only packages published by Codebelt-related source repositories. The previous approach of querying NuGet API for latest versions of all packages has been removed, and third-party packages (Microsoft.Extensions.*, BenchmarkDotNet, etc.) are now explicitly excluded from automatic updates. Additionally, automated Docker image version bumps have been removed due to issues with incorrect variant selection.
Changes:
- Rewrote
.github/scripts/bump-nuget.pyto only update packages from the triggering source repository using the provided TRIGGER_VERSION, removing all NuGet API queries for third-party packages - Removed Docker image version bumping logic from the workflow (test runner and NGINX Alpine)
- Updated PR body template to clarify that only Codebelt/Cuemon packages are auto-updated and third-party packages should be updated via Dependabot or manually
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/scripts/bump-nuget.py |
Complete rewrite to simplify package bumping logic - now only updates packages published by the triggering source repo (e.g., Codebelt, Cuemon, Savvyio) using the provided TRIGGER_VERSION; removed NuGet API queries; improved error handling and result reporting |
.github/workflows/service-update.yml |
Removed automated Docker image version bumps for test runners and NGINX Alpine with explanatory comment; updated PR body template to reflect that only Codebelt/Cuemon packages are auto-bumped and third-party packages require Dependabot or manual updates |
| pattern = re.compile( | ||
| r"<PackageVersion\b" | ||
| r'(?=[^>]*\bInclude="([^"]+)")' | ||
| r'(?=[^>]*\bVersion="([^"]+)")' | ||
| r"[^>]*>", | ||
| re.DOTALL, | ||
| ) |
There was a problem hiding this comment.
The regex pattern correctly uses re.DOTALL to match multiline PackageVersion elements, but it will match ALL PackageVersion elements regardless of ItemGroup conditions (e.g., those within Condition="$(TargetFramework.StartsWith('net9'))" blocks). This means if a Cuemon package appears in multiple conditional ItemGroups with different versions for different target frameworks, they will ALL be updated to the same TRIGGER_VERSION. This may be intentional per the script's documentation ("Does NOT parse TFM conditions"), but could lead to issues if different TFMs should have different package versions. Consider documenting this behavior more explicitly or adding a warning if conditional ItemGroups are detected.
| print(f" TRIGGER_SOURCE={TRIGGER_SOURCE}") | ||
| print(f" TRIGGER_VERSION={TRIGGER_VERSION}") | ||
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
When TRIGGER_SOURCE is provided but doesn't exist in SOURCE_PACKAGE_MAP, the function is_triggered_package will return False for all packages (since prefixes will be an empty list from .get(TRIGGER_SOURCE, [])). This means the script will skip all packages and produce no changes. While this is safe, it would be better to add validation at the start of main() to check if TRIGGER_SOURCE exists in SOURCE_PACKAGE_MAP and exit with a clear error message if not. This would help catch configuration errors early.
| if TRIGGER_SOURCE not in SOURCE_PACKAGE_MAP: | |
| print( | |
| f"Error: Unknown TRIGGER_SOURCE '{TRIGGER_SOURCE}'. " | |
| f"Expected one of: {', '.join(SOURCE_PACKAGE_MAP.keys())}" | |
| ) | |
| sys.exit(1) |
| .docfx/Dockerfile.docfx && echo "Bumped NGINX to $LATEST" | ||
| # Note: Docker image bumps removed in favor of manual updates | ||
| # The automated selection was picking wrong variants (e.g., mono-* instead of standard) | ||
| # TODO: Move to hosted service for smarter image selection |
There was a problem hiding this comment.
The comment says "smarter image selection" but doesn't specify what "smarter" means. Consider being more specific about what improvements are needed, such as "image selection that filters by architecture and excludes variant tags like mono-*" or "automated selection with maintainer-defined filter rules". This will help whoever implements the hosted service understand the requirements.
| # TODO: Move to hosted service for smarter image selection | |
| # TODO: Move to a hosted service that selects Docker images by architecture, excludes mono-* and other unwanted variants, and applies maintainer-defined filter rules for tag selection. |
| current = m.group(2) | ||
| # Strip 'v' prefix if present in version | ||
| target_version = TRIGGER_VERSION.lstrip("v") | ||
|
|
There was a problem hiding this comment.
Consider adding validation for TRIGGER_VERSION format to ensure it follows semantic versioning (e.g., "1.2.3" or "1.2.3-beta"). Invalid version strings could be written to Directory.Packages.props, potentially breaking builds. A simple regex check like re.match(r'^\d+\.\d+\.\d+(-[\w.]+)?$', target_version) would catch obvious mistakes early.
| # Validate that the target version looks like a semantic version (e.g. 1.2.3 or 1.2.3-beta) | |
| if not re.match(r"^\d+\.\d+\.\d+(-[\w.]+)?$", target_version): | |
| print(f"Error: Invalid TRIGGER_VERSION format: {TRIGGER_VERSION!r}") | |
| print(" Expected a semantic version like '1.2.3' or '1.2.3-beta'.") | |
| sys.exit(1) |
| with open("Directory.Packages.props", "w") as f: | ||
| f.write(new_content) | ||
|
|
||
| return 0 if changes else 0 # Return 0 even if no changes (not an error) |
There was a problem hiding this comment.
The return statement is redundant since both branches return 0. This could be simplified to just return 0.
| return 0 if changes else 0 # Return 0 even if no changes (not an error) | |
| return 0 # Return 0 even if no changes (not an error) |
1 participant
This pull request simplifies and restricts the NuGet package version bumping process in the service update workflow. The new approach ensures that only packages published by the triggering Codebelt-related source repository are updated, while third-party packages (such as Microsoft.Extensions.* and BenchmarkDotNet) are excluded from automatic updates. Additionally, automated Docker image version bumps have been removed from the workflow.
Key changes include:
NuGet package bumping simplification:
.github/scripts/bump-nuget.py: Rewritten to only update packages published by the triggering source repo (e.g., Codebelt, Cuemon, Savvyio), using the providedTRIGGER_VERSION. Third-party packages are explicitly skipped, and the script no longer queries the NuGet API for the latest version of non-Codebelt packages. Error handling and result reporting are improved.Workflow and documentation updates:
.github/workflows/service-update.yml: Removed automated Docker image version bumps for test runners and NGINX Alpine, with a note explaining the rationale and a TODO for smarter image selection in the future..github/workflows/service-update.yml: Updated the pull request body template to clarify that only Codebelt/Cuemon package versions are bumped automatically, and added a note that third-party packages should be updated via Dependabot or manually.Summary by CodeRabbit