Skip to content

Conversation

@F-X64
Copy link
Collaborator

@F-X64 F-X64 commented Jan 28, 2026

TBD

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • In nvrGreaterOrEqual, any unexpected rpmdev-vercmp exit code (e.g., errors not equal to 0/11/12) is currently treated as success; consider explicitly handling non-comparison error codes to avoid silently passing invalid comparisons.
  • The new error_handler combined with set -e and trap ERR may make failure handling harder to reason about (especially in subshells or pipelines); consider using set -E and/or limiting the trap scope so that only relevant failures are reported.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `nvrGreaterOrEqual`, any unexpected `rpmdev-vercmp` exit code (e.g., errors not equal to 0/11/12) is currently treated as success; consider explicitly handling non-comparison error codes to avoid silently passing invalid comparisons.
- The new `error_handler` combined with `set -e` and `trap ERR` may make failure handling harder to reason about (especially in subshells or pipelines); consider using `set -E` and/or limiting the trap scope so that only relevant failures are reported.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@alexxa
Copy link
Collaborator

alexxa commented Jan 28, 2026

WOW!

@F-X64 F-X64 force-pushed the fix-nvr-version-comparison branch 4 times, most recently from 362855d to ba33704 Compare February 3, 2026 09:56
Remove ENV escaping for late evaluation
Immediate evaluation used for TOML / AWSCLI parameters
@F-X64 F-X64 force-pushed the fix-nvr-version-comparison branch from ba33704 to f8f6ff5 Compare February 3, 2026 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants