rules_img support

[apesternikov]

Description

This PR implements rules_img support while keeping compatibility with rules_oci to facilitate transition

How Has This Been Tested?

unit test for rules_img has been implemented. Also, verifying end-to-end with external project.

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.

[codereviewbot-ai]

Potential runtime error due to lack of validation after assigning gitServer

After the switch statement, there is no check to ensure that gitServer is valid (i.e., not nil or a non-function). If git.ServerFunc returns nil or an invalid function, subsequent calls to gitServer.CreatePR may panic. To improve robustness, add a validation step after assignment:

if gitServer == nil {
    log.Fatalf("Failed to initialize git server implementation for host: %s", *gitHost)
}

This ensures that any issues with the assignment are caught early, preventing unexpected runtime errors.

[codereviewbot-ai]

Diagnostic Limitation in Verification Loop

The verification loop for gitops targets (starting at line 67) exits immediately upon encountering the first missing target. This approach prevents the script from reporting all missing targets, which can hinder debugging and reduce diagnostic value.

Recommended Solution:

Modify the loop to collect all missing targets and report them together at the end, for example:

missing_targets=()
for target in "${EXPECTED_TARGETS[@]}"; do
  if ! grep -q "target $target" "$OUTPUT_FILE"; then
    missing_targets+=("$target")
  fi
done
if [ ${#missing_targets[@]} -ne 0 ]; then
  echo "ERROR: The following gitops targets were not found in output:" >&2
  printf '  %s\n' "${missing_targets[@]}" >&2
  exit 1
fi

This approach improves maintainability and provides more comprehensive feedback.

[codereviewbot-ai]

Race Condition: Registry Readiness
The script uses a fixed sleep 1.5 to wait for the registry to start, which may not reliably ensure the registry is ready to accept connections. This can lead to intermittent test failures if the registry takes longer to start.

Recommendation:
Replace the fixed sleep with a loop that checks the registry's readiness, e.g.:

for i in {1..10}; do
  if nc -z localhost 1338; then
    break
  fi
  sleep 0.5
done

This ensures the test proceeds only when the registry is actually ready.

[codereviewbot-ai]

Error Handling: Push and Validation Steps
The script does not check the exit codes of the push operation ($1) or the validation step (${CRANE_BIN} validate). If either fails, the script will continue and may report a false positive.

Recommendation:
Add error checks after each critical step:

$1 || { echo 'Push failed'; exit 1; }
${CRANE_BIN} validate ... || { echo 'Validation failed'; exit 1; }

This ensures failures are detected and reported promptly.

[codereviewbot-ai]

Unreliable registry startup wait

Using sleep 1.5 to wait for the registry to start is not robust, as startup time may vary. Replace with a loop that checks registry readiness, e.g.:

for i in {1..10}; do
  if curl -s localhost:1338/v2/ > /dev/null; then
    break
  fi
  sleep 0.5
done

This ensures the script proceeds only when the registry is actually ready.

[codereviewbot-ai]

Missing error handling for push and validation steps

The script does not check the exit status of the push rule target ($1) or the crane validation. Add error checks after each step to ensure failures are detected:

$1 || { echo "Push rule target failed"; exit 1; }
${CRANE_BIN} validate -v --fast --remote localhost:1338/repo/oci_img:testtag || { echo "Image validation failed"; exit 1; }

This will make failures explicit and prevent false positives.

[michaelschiff]

tested in my repos still using rules_oci - LGTM

[codereviewbot-ai]

Trap Overwrite May Cause Incomplete Cleanup

The trap command is set twice: first on line 13 to kill the registry, then on line 25 to remove the temp directory and kill the registry. The second trap overwrites the first, and if the temp directory removal fails or is executed first, the registry process may not be properly killed, potentially leaving orphaned processes.

Recommended Solution:
Combine cleanup actions into a single trap, e.g.:

trap 'rm -rf "$tmp_dir"; kill -9 $registry_pid' EXIT

Set this after both $tmp_dir and $registry_pid are initialized.

[codereviewbot-ai]

Lack of Error Checking for Critical External Commands

The script starts the registry (${REGISTRY_BIN}) and executes the apply target (${apply_bin}) without checking their exit statuses. If either fails, subsequent steps may produce misleading results or fail silently.

Recommended Solution:
Add error checks after these commands:

${REGISTRY_BIN} &
registry_pid=$!
if ! kill -0 $registry_pid 2>/dev/null; then
  echo "Error: Registry failed to start"
  exit 1
fi
...
${apply_bin}
if [ $? -ne 0 ]; then
  echo "Error: apply_bin failed"
  exit 1
fi

This ensures failures are detected early and reported.