Enhance versions_map generator logic #741
Conversation
WalkthroughThis pull request updates the commit validation logic in the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Script
participant ChartYAML as Chart.yaml
participant Tags
User->>Script: Run `gen_versions_map.sh` with commit value
alt Commit is not HEAD
Script->>ChartYAML: Validate commit against expected version
alt Commit valid
Script->>User: Output version map
else Commit invalid
Script->>User: Print error and exit
end
else Commit is HEAD
Script->>ChartYAML: Check for version presence
alt Version found in Chart.yaml
Script->>User: Output version map
else Version missing
Script->>Tags: Search for valid commit matching version
alt Valid commit found
Script->>User: Output version map
else No valid commit found
Script->>User: Print error and exit
end
end
end
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/apps/versions_map (1)
2-157: 💡 Verification agent🧩 Analysis chain
Comprehensive update of commit references for application packages
The changes systematically update commit hashes for numerous application packages including clickhouse, ferretdb, http-cache, kafka, kubernetes, mysql, nats, postgres, rabbitmq, redis, tcp-balancer, tenant, virtual-machine, vm-disk, vm-instance, and vpn.
This is a well-structured update that ensures consistent version tracking across all application packages.
🏁 Script executed:
#!/bin/bash # Verify a sample of the updated commit hashes to ensure they exist # and have the correct Chart.yaml version # Sample verification for kubernetes package echo "Checking kubernetes 0.15.2 (8267072d)..." if git show 8267072d:./kubernetes/Chart.yaml 2>/dev/null | grep -q "^version: 0.15.2$"; then echo "✓ kubernetes 0.15.2 verified" else echo "✗ kubernetes 0.15.2 not verified" fi # Sample verification for virtual-machine package echo "Checking virtual-machine 0.7.1 (0ab39f20)..." if git show 0ab39f20:./virtual-machine/Chart.yaml 2>/dev/null | grep -q "^version: 0.7.1$"; then echo "✓ virtual-machine 0.7.1 verified" else echo "✗ virtual-machine 0.7.1 not verified" fi # Check if there are any duplicate entries that should be consolidated echo "Checking for duplicate package-version combinations..." sort packages/apps/versions_map | uniq -d -cLength of output: 665
Action Required: Resolve Inconsistent Commit References in versions_map
During verification, the following discrepancies were identified:
- kubernetes (v0.15.2 / commit 8267072): The
Chart.yamlfile does not reflect the expected version string (version: 0.15.2).- virtual-machine (v0.7.1 / commit 0ab39f2): The
Chart.yamlfile does not contain the expected version (version: 0.7.1).Please review and update either the commit hashes in
packages/apps/versions_mapor the corresponding version strings in theChart.yamlfiles to ensure consistency in version tracking across packages.
🛑 Comments failed to post (1)
packages/extra/versions_map (1)
1-42: 💡 Verification agent
🧩 Analysis chain
Consistent update of commit hashes for package versions
The systematic update of commit hashes for bootbox, etcd, ingress, monitoring, and seaweedfs packages ensures that each version points to the correct commit. This aligns with the improved validation logic in the gen_versions_map.sh script.
These changes don't alter the version numbers themselves, just the commit references, indicating this is likely a maintenance update to ensure accurate version tracking.
🏁 Script executed:
#!/bin/bash # Verify that the updated commit hashes actually exist in the repository # and have the expected Chart.yaml version for a sample of packages # Check for bootbox 0.1.0 echo "Checking bootbox 0.1.0 (45a7416c)..." if git show 45a7416c:./bootbox/Chart.yaml 2>/dev/null | grep -q "^version: 0.1.0$"; then echo "✓ bootbox 0.1.0 verified" else echo "✗ bootbox 0.1.0 not verified" fi # Check for etcd 2.6.0 echo "Checking etcd 2.6.0 (8c460528)..." if git show 8c460528:./etcd/Chart.yaml 2>/dev/null | grep -q "^version: 2.6.0$"; then echo "✓ etcd 2.6.0 verified" else echo "✗ etcd 2.6.0 not verified" fi # Check for monitoring 1.9.0 echo "Checking monitoring 1.9.0 (45a7416c)..." if git show 45a7416c:./monitoring/Chart.yaml 2>/dev/null | grep -q "^version: 1.9.0$"; then echo "✓ monitoring 1.9.0 verified" else echo "✗ monitoring 1.9.0 not verified" fiLength of output: 659
Action Required: Correct Commit Hash Mappings for Package Versions
The verification script indicates that the commit hashes for several packages do not match the expected version strings in their respective Chart.yaml files:
- bootbox 0.1.0 (45a7416): Expected
version: 0.1.0was not found.- etcd 2.6.0 (8c46052): Expected
version: 2.6.0was not verified.- monitoring 1.9.0 (45a7416): Expected
8000version: 1.9.0is missing in the commit.Please revisit these commit references to ensure they point to commits with the correct Chart.yaml version definitions, and update the versions map accordingly if necessary.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
hack/gen_versions_map.sh (4)
10-10: AWK Extraction Logic for Chart DataThe AWK command extracting the chart name and version (defaulting the commit to "HEAD") is concise and works for simple YAML files. However, note that it assumes a very specific format for
Chart.yaml(with keys starting at the beginning of the line and exactly one token following).
19-19: Missing Mapping CalculationThe use of AWK here to build an associative array (
nm) and then compare the new mappings against the existing file is clever and succinct. It correctly identifies mappings that differ.
Consider adding an inline comment to explain the logic for future maintainability, and ensure that the format of both inputs remains consistent.
21-24: Tag Retrieval and Commit Search EnhancementThe new block that retrieves tags (via
git ls-remote) and appends the latest main commit is a solid enhancement. Please verify that your filtering pattern (refs/tags/v) and sorting (sort -k2,2 -rV) meet all expected tag-naming conventions in your repository. If tags might deviate from the expected format, consider whether additional filtering or error handling is needed.
35-36: Shell Variable Quoting in Conditional CheckIn the condition checking if
commitis not"HEAD", the variable is unquoted. This can lead to word splitting or globbing issues if the variable ever contains spaces or special characters.
Please change it to:-if [ $commit != "HEAD" ]; then +if [ "$commit" != "HEAD" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hack/gen_versions_map.sh(2 hunks)packages/apps/versions_map(1 hunks)packages/extra/versions_map(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extra/versions_map
- packages/apps/versions_map
🔇 Additional comments (1)
hack/gen_versions_map.sh (1)
64-64: Final Map OutputThe final output construction using
printfpiped intosortandawkis neat and clearly sorts mappings by chart and version. The use ofawk '$1'effectively filters out any blank lines. This segment looks good assuming consistent formatting of inputs.
| fi | ||
| echo "$miss_map" | while read -r chart version commit; do | ||
| # if version is found in HEAD, it's HEAD | ||
| if grep -q "^version: $version$" ./${chart}/Chart.yaml; then |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Grep Pattern Robustness
The grep usage to match the version in Chart.yaml (e.g.,
grep -q "^version: $version$" ./${chart}/Chart.yaml) works under the assumption that the version string contains no regex metacharacters. To avoid potential issues, consider using fixed-st
8000
ring matching with grep -F or properly quoting/escaping the variable.
Grep Pattern Robustness: Ensure Fixed-String Matching
At hack/gen_versions_map.sh:29, the current grep command:
if grep -q "^version: $version$" ./${chart}/Chart.yaml; thenassumes that the $version variable contains no regex metacharacters. This can lead to potential mismatches (for instance, dots in version numbers being interpreted as any character). To mitigate this risk, please refactor the code to either:
- Use fixed-string matching by adding the
-Fflag (e.g.,grep -F -q), or - Escape the
$versionvariable appropriately before incorporating it into the regex.
| # if commit is HEAD, but version is not found in HEAD, check all tags | ||
| found_tag="" | ||
| for tag in $search_commits; do | ||
| if git show "${tag}:./${chart}/Chart.yaml" 2>/dev/null | grep -q "^version: $version$"; then |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consistent Grep Usage for Tag-based Lookup
Similarly, the grep command used when iterating over tags (line 49) should be reviewed for the same potential regex pitfalls. Changing it to a fixed-string search might increase robustness.
Update grep Invocation for Fixed-String Matching
- File:
hack/gen_versions_map.sh(line 49) - Issue: The current command
uses a regex to match the version string. If the variable
if git show "${tag}:./${chart}/Chart.yaml" 2>/dev/null | grep -q "^version: $version$"; then
$versionever contains any regex metacharacters, this could lead to unintended behavior. - Suggestion: Replace the regex search with a fixed-string match. For instance, updating the line to:
ensures that the content is compared exactly as a literal string, which increases robustness when matching tag-based version lookups.
if git show "${tag}:./${chart}/Chart.yaml" 2>/dev/null | grep -F -x -q "version: $version"; then
Summary by CodeRabbit