fix: get resolves pinned version instead of defaulting to latest tag#479
fix: get resolves pinned version instead of defaulting to latest tag#479jpower432 wants to merge 2 commits intocomplytime:mainfrom
Conversation
CheckPolicyVersions always resolved the `latest` tag to check for staleness. Registries without a `latest` tag (but with the pinned version present) were misreported as "unreachable". Now falls back to resolving the pinned version as a reachability check before declaring a registry down, and surfaces the actual error when the registry is truly unreachable. Made-with: Cursor Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
SyncPolicy passed the bare policyID to DefinitionVersion, which unconditionally appended :latest. Registries without a latest tag failed even when the pinned version existed. DefinitionVersion now only appends :latest when the modulePath has no explicit version, and SyncPolicy constructs policyID:version when a version is specified. Made-with: Cursor Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
jpower432
changed the title
marcusburghardt
left a comment
marcusburghardt
left a comment
There was a problem hiding this comment.
AI-assisted review: 2 test coverage gaps identified (CRAP regressions), 1 positive note on doctor tests.
| ref := fmt.Sprintf("%s/%s:latest", c.registryHost(), modulePath) | ||
| ref := fmt.Sprintf("%s/%s", c.registryHost(), modulePath) | ||
| if !strings.Contains(modulePath, ":") && !strings.Contains(modulePath, "@") { | ||
| ref += ":latest" |
There was a problem hiding this comment.
[HIGH] CRAP regression: DefinitionVersion crossed CRAPload threshold (6.7 → 15.3)
The new strings.Contains branching (lines 56-58) is untested — all existing tests in client_test.go use NewClientWithFetcher with a mock, which returns at the c.fetcher != nil guard (line 52) before reaching this logic.
Consider adding tests that use a Client without a fetcher (or with a nil fetcher) and exercise three paths:
modulePathwithout:or@→ should append:latestmodulePathwith:(e.g.,"policies/nist:v1.0.0") → should NOT append:latestmodulePathwith@(e.g.,"policies/nist@sha256:abc") → should NOT append:latest
Since these hit a real registry resolve, you'd likely need to test at the ref-construction level (e.g., extract a buildRef helper that's independently testable) or use fetchVersion with a test server.
| lookupRef := policyID | ||
| if version != "" && version != "latest" { | ||
| lookupRef = policyID + ":" + version | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing test coverage for version-specific ref construction
The new branching (version != "" && version != "latest" → policyID + ":" + version) is not exercised by existing sync tests — they all call SyncPolicy with "latest". CRAP score rose from 11.4 → 13.9.
A test like TestSync_CopyOnSuccess_PinnedVersion calling SyncPolicy(ctx, "test-policy", "v1.0.0") and verifying that the source receives the "test-policy:v1.0.0" ref would cover this path.
| if results[0].Status != StatusWarn { | ||
| t.Errorf("expected warn, got %s", results[0].Status) | ||
| } | ||
| if !strings.Contains(results[0].Message, "connection refused") { |
There was a problem hiding this comment.
[INFO] Good coverage — two new test cases cover the pinned-version fallback
TestCheckPolicyVersions_LatestMissing_PinnedResolves (positive) and TestCheckPolicyVersions_LatestMissing_NoPinnedVersion (negative) properly verify both outcomes of the new fallback logic. The existing TestCheckPolicyVersions_RegistryUnreachable was also updated to assert the actual error message. This is the model the other changed files should follow.
2 participants
Summary
SyncPolicy passed the bare policyID to DefinitionVersion, which unconditionally appended :latest. Registries without a latest tag failed even when the pinned version existed. DefinitionVersion now only appends :latest when the modulePath has no explicit version, and SyncPolicy constructs policyID:version when a version is specified.
Related Issues
N/A
Review Hints
Add a version the policy ref in the
complytime.yaml(example:@v1.0.0). It currently fails and appends:latestto it. With this change@syntax should be fully supported.