Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPinned multiple GitHub Actions to specific commit SHAs, bumped Go toolchain to 1.25, added .gitignore entries, refactored image-building script into helpers, and rewrote command parsing and test grouping in tests/main.go. Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/test_images.yaml (1)
102-103: Pingo-versionto1.25.8to match the module specification and ensure deterministic builds.The module explicitly targets
go 1.25.8(tests/go.mod), but the workflow usesgo-version: '1.25', which may resolve to different patch versions over time and create version drift, particularly concerning given the CVE remediation context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test_images.yaml around lines 102 - 103, The workflow currently uses the broad key go-version: '1.25' which can resolve to different patch releases; update the go-version value to the exact patched version used in tests (change go-version from '1.25' to '1.25.8') so the CI uses the deterministic Go release that matches tests/go.mod.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build_pgedge_images.py`:
- Around line 334-338: The dry-run path calls index_digest(image.build_tag) and
sign(...) even when build(...) skipped pushing, which can fail; update the logic
in the conditional around build(repo=..., image=..., dry_run=...) so that
index_digest and sign are only invoked when an actual push happened (e.g., when
not config.dry_run and either the image was already published or republish
occurred), or alternatively guard the index_digest and sign calls with a check
for not config.dry_run (and/or confirmed publish), referencing the build,
index_digest, and sign calls to locate and change the flow.
In `@tests/main.go`:
- Around line 543-545: The current tokenization returns []string{cmd} when
parseCommand yields no args, which misrepresents whitespace-only input; modify
parseCommand (or the place where p.args is returned) to return an empty slice
instead of []string{cmd}, and add a guard in TestRunner.exec right after calling
parseCommand (check len(cmdArgs) == 0) to fail fast by returning -1, "",
fmt.Errorf("empty command") so whitespace-only commands are rejected early;
reference parseCommand, p.args (or cmdArgs) and TestRunner.exec to locate the
changes.
---
Nitpick comments:
In @.github/workflows/test_images.yaml:
- Around line 102-103: The workflow currently uses the broad key go-version:
'1.25' which can resolve to different patch releases; update the go-version
value to the exact patched version used in tests (change go-version from '1.25'
to '1.25.8') so the CI uses the deterministic Go release that matches
tests/go.mod.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10e2e4b8-fa00-40da-84d3-a83325a0301e
⛔ Files ignored due to path filters (1)
tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.github/workflows/build_images.yaml.github/workflows/pr_test_latest.yaml.github/workflows/test_images.yaml.gitignorescripts/build_pgedge_images.pytests/go.modtests/main.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/build_pgedge_images.py (1)
335-335:⚠️ Potential issue | 🟠 Major
no_cache/only_archare still not applied during build.Even though Line 335 passes both flags,
build()currently computesbake_argsbut executesbake_cmd("--push"), so those options are ignored.Root-cause fix in
build()- subprocess.check_output( - bake_cmd("--push"), + subprocess.check_output( + bake_cmd(*bake_args), env={ **os.environ.copy(), "PACKAGE_RELEASE_CHANNEL": image.package_release_channel, "POSTGRES_MAJOR_VERSION": image.postgres_major, "PACKAGE_LIST_FILE": image.package_list, "TAG": f"{repo}:{image.build_tag}", "TARGET": image.flavor, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_pgedge_images.py` at line 335, The build() function computes bake_args (including no_cache and only_arch) but then ignores them by calling bake_cmd("--push") directly; update build() so the computed bake_args are actually passed into the bake execution (e.g., call bake_cmd with "--push" plus the bake_args array) and ensure the logic that appends flags for no_cache and only_arch to bake_args (the variables named bake_args and bake_cmd in the diff) is correct and used; locate the build(...) function and modify the invocation to include bake_args so the no_cache and only_arch options are applied during the build.
🧹 Nitpick comments (1)
scripts/build_pgedge_images.py (1)
300-313: Clarifyonly_archlog wording.Line 312 says “other images will be skipped,” but
only_archconstrains platforms, not image selection. This can mislead operators during runs.Suggested wording tweak
- if config.only_arch: - logging.info(f"only arch {config.only_arch} enabled. other images will be skipped.") + if config.only_arch: + logging.info(f"only arch {config.only_arch} enabled. builds will target linux/{config.only_arch} only.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_pgedge_images.py` around lines 300 - 313, The log message for config.only_arch in _log_config is misleading — it says "other images will be skipped" though only_arch restricts platforms, not which images are selected; update the message in the _log_config function (where config.only_arch is referenced) to clearly state that platform/architecture selection is constrained (e.g., "only arch X enabled. builds will be limited to that architecture/platform"), keeping the other config logs unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/build_pgedge_images.py`:
- Line 335: The build() function computes bake_args (including no_cache and
only_arch) but then ignores them by calling bake_cmd("--push") directly; update
build() so the computed bake_args are actually passed into the bake execution
(e.g., call bake_cmd with "--push" plus the bake_args array) and ensure the
logic that appends flags for no_cache and only_arch to bake_args (the variables
named bake_args and bake_cmd in the diff) is correct and used; locate the
build(...) function and modify the invocation to include bake_args so the
no_cache and only_arch options are applied during the build.
---
Nitpick comments:
In `@scripts/build_pgedge_images.py`:
- Around line 300-313: The log message for config.only_arch in _log_config is
misleading — it says "other images will be skipped" though only_arch restricts
platforms, not which images are selected; update the message in the _log_config
function (where config.only_arch is referenced) to clearly state that
platform/architecture selection is constrained (e.g., "only arch X enabled.
builds will be limited to that architecture/platform"), keeping the other config
logs unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d09b5f3e-2bcc-419c-96ca-1504c9b9700b
📒 Files selected for processing (1)
scripts/build_pgedge_images.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build_pgedge_images.py`:
- Around line 323-327: In _process_extra_tags, the current check uses
tag_published.issubset(published) which wrongly treats partial-platform digests
as up-to-date; instead compare exact digest sets. Change the conditional that
decides to call add_tag (in function _process_extra_tags, using
published_digests and variables tag_published and published) to require
equality: if tag_published is empty OR tag_published != published OR
config.republish then call add_tag so stale/partial tags are retagged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc10276a-1045-4afa-90e5-e5f545f87a15
📒 Files selected for processing (1)
scripts/build_pgedge_images.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build_pgedge_images.py`:
- Line 258: The bake command override is using the wrong key name: change the
override from "default.platforms=linux/{only_arch}" to the supported
"default.platform=linux/{only_arch}" so the Buildx bake --set platform override
takes effect; locate the call building bake_args (referenced by bake_cmd and the
only_arch variable) and replace the plural platforms key with the singular
platform key in the --set override string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60f0e766-7cc8-493f-8743-4fe3c6389018
📒 Files selected for processing (1)
scripts/build_pgedge_images.py
Use singular instead of in the --set override; is the HCL bake-file attribute, but the CLI override key for docker buildx bake --set is .
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
This looks good to me! My comment is non-blocking, but in general, I think we should avoid refactoring just based on complexity/function length. If refactoring makes the code more modular, encourages reuse, improves patterns, etc. then that's worthwhile. But, otherwise, I'd just ignore those results.
| } | ||
| } | ||
|
|
||
| // parseCommand safely parses a command string into command and arguments |
There was a problem hiding this comment.
Non-blocking: I prefer the way this was before. Cyclomatic complexity and function length are useful indicators that the code deserves a closer look, but I don't like to act on them in the absence of other problems. Codacy and similar tools treat them like proxies for "readability" and "maintainability", but those are subjective and seemingly unquantifiable. I think they're wrong to encourage everyone to refactor code solely to reduce those two metrics.
2 participants
False Positives Marked