Merged
Conversation
Contributor
|
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:
WalkthroughAdds CEL functions exposing config access and logs; refactors external-entity remapping to use separate user/group ID maps and inserts remapped groups via a temp table; extends e2e pre-population to create ConfigAccess and ConfigAccessLog records; adds unit/integration/e2e tests; updates Dockerfile and Go module deps. Changes
Sequence Diagram(s)sequenceDiagram
participant E2E as E2E Test / Scraper
participant Prepop as Pre-population
participant DB as Database
participant Merge as merge_and_upsert
participant Logs as ConfigAccess/Logs
rect rgba(200,200,255,0.5)
E2E->>Prepop: create fixtures (configs, external users/groups/roles, config_access, config_access_logs)
Prepop->>DB: insert pre-populated rows
end
rect rgba(200,255,200,0.5)
E2E->>DB: insert remapped bridge rows into temp table
DB->>Merge: call merge_and_upsert_external_entities(temp table)
Merge->>DB: resolve winner/loser, apply upserts/deletions
Merge->>Logs: consolidate/merge ConfigAccess and ConfigAccessLog entries
Logs->>DB: write aggregated logs and update config_access references
DB-->>E2E: return merge mappings/results
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
moshloop
force-pushed
the
pr/fix-external-merge-error
branch
3 times, most recently
from
1882a7b to
3571ee0
Compare
moshloop
force-pushed
the
pr/fix-external-merge-error
branch
from
3571ee0 to
2a2437e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
db/cel_functions.go (1)
150-162: Consider:queryConfigAccessLogshas nodeleted_atfilter.Unlike
queryEntitieswhich supportsincludeDeleted, this function does not filter bydeleted_at. Ifconfig_access_logshas soft-delete semantics, you may want to add a similar filtering capability for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/cel_functions.go` around lines 150 - 162, The function queryConfigAccessLogs currently always returns all rows and lacks soft-delete handling; modify queryConfigAccessLogs to accept an includeDeleted boolean (or use an existing includeDeleted from the caller) and apply conditional filtering on the DB query: when includeDeleted is false, add a condition to exclude soft-deleted rows (e.g., Where("deleted_at IS NULL") or omit Unscoped), and when includeDeleted is true, use Unscoped() or remove that condition so deleted rows are included; update the DB call that builds the query (the ctx.DB().Table("config_access_logs").Where(...).Find(...) chain) accordingly and propagate the new parameter through callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/Dockerfile`:
- Line 8: The Dockerfile's build stage is missing the "builder" alias referenced
by COPY --from=builder; update the initial build stage declaration by adding the
stage name "builder" to the FROM that currently reads the Go base image so
subsequent COPY --from=builder operations can resolve the stage (target the FROM
golang:1.25.8-bookworm@sha256:2630d37044ad6b9c5acfc343918cefefa508b2a853f1a316ceda8600b6dd12c7
line and add the AS builder alias).
In `@scrapers/external_entity_merge_test.go`:
- Around line 104-134: The current teardown defer runs only after setup
completes, which risks leaking created records and leaving transactions open on
failure; fix by registering cleanup immediately after each successful create
using the test framework's DeferCleanup for the created records (e.g., the
created ConfigItem, scraperModel, ExternalUser entries like bridge with ID
bridgeID and any access logs using DefaultContext.DB().Create) and, right after
calling tx := DefaultContext.DB().Begin(), register a rollback cleanup that
calls tx.Rollback() (or tx.RollbackUnlessCommitted) to ensure the transaction is
closed on test failure; keep the existing final deletions as an extra cleanup
but move/augment record-specific cleanup to immediately follow each successful
insert (and register tempTable cleanup if applicable) so no resources leak on
early failures.
In `@scrapers/extract_e2e_test.go`:
- Around line 205-209: The test currently uses
configIDByExternalID[ca.ConfigExternalID] which returns a zero UUID for unknown
keys; instead, explicitly look up the key (e.g. id, ok :=
configIDByExternalID[ca.ConfigExternalID]) before constructing
dutymodels.ConfigAccess and fail fast if not found (t.Fatalf or return an error)
with a clear message including ca.ConfigExternalID so a bad fixture reference is
reported directly rather than causing a generic FK insert failure.
- Around line 232-243: The loop over fixture.PrePopulate.ConfigAccessLogs must
validate inputs before using them: ensure each log has at least one
ExternalUserAliases entry (check len(log.ExternalUserAliases) > 0) and assert
the alias maps via userIDByAlias, and assert the ConfigExternalID maps via
configIDByExternalID (and is not the zero/empty UUID) instead of indexing or
falling back; use Expect(...) to fail the test with clear messages referencing
fixture.PrePopulate.ConfigAccessLogs, log.ExternalUserAliases, userIDByAlias,
and configIDByExternalID so bad fixtures produce assertions rather than panics
or FK errors.
---
Nitpick comments:
In `@db/cel_functions.go`:
- Around line 150-162: The function queryConfigAccessLogs currently always
returns all rows and lacks soft-delete handling; modify queryConfigAccessLogs to
accept an includeDeleted boolean (or use an existing includeDeleted from the
caller) and apply conditional filtering on the DB query: when includeDeleted is
false, add a condition to exclude soft-deleted rows (e.g., Where("deleted_at IS
NULL") or omit Unscoped), and when includeDeleted is true, use Unscoped() or
remove that condition so deleted rows are included; update the DB call that
builds the query (the ctx.DB().Table("config_access_logs").Where(...).Find(...)
chain) accordingly and propagate the new parameter through callers.
🪄 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: d679b686-7cf0-4245-bf82-3ee0befd2824
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
build/Dockerfiledb/cel_functions.godb/external_entities.godb/external_entities_test.gofixtures/data/external_roles_merge_bridge.jsongo.modscrapers/external_entity_merge_test.goscrapers/extract/testdata/e2e/external_groups_merge_config_access_collision.yamlscrapers/extract/testdata/e2e/external_roles_merge_config_access_collision.yamlscrapers/extract/testdata/e2e/external_users_merge_access_logs_collision.yamlscrapers/extract/testdata/e2e/external_users_merge_config_access_collision.yamlscrapers/extract_e2e_test.go
Comment on lines
+104
to
+134
| defer func() { | ||
| Expect(DefaultContext.DB().Where("config_id = ? AND scraper_id = ?", configID, scraperModel.ID).Delete(&dutymodels.ConfigAccessLog{}).Error).NotTo(HaveOccurred()) | ||
| Expect(DefaultContext.DB().Where("config_id = ? AND scraper_id = ?", configID, scraperModel.ID).Delete(&dutymodels.ConfigAccess{}).Error).NotTo(HaveOccurred()) | ||
| Expect(DefaultContext.DB().Where("scraper_id = ?", scraperModel.ID).Delete(&dutymodels.ExternalUser{}).Error).NotTo(HaveOccurred()) | ||
| Expect(DefaultContext.DB().Delete(&models.ConfigItem{}, "id = ?", configItem.ID).Error).NotTo(HaveOccurred()) | ||
| Expect(DefaultContext.DB().Delete(&scraperModel).Error).NotTo(HaveOccurred()) | ||
| }() | ||
|
|
||
| tx := DefaultContext.DB().Begin() | ||
| Expect(tx.Error).NotTo(HaveOccurred()) | ||
|
|
||
| tempTable := fmt.Sprintf("_merge_users_%s", strings.ReplaceAll(uuid.NewString(), "-", "_")) | ||
| Expect(tx.Exec(fmt.Sprintf(`CREATE TEMP TABLE %s (LIKE external_users INCLUDING ALL) ON COMMIT DROP`, tempTable)).Error).NotTo(HaveOccurred()) | ||
|
|
||
| bridge := dutymodels.ExternalUser{ | ||
| ID: bridgeID, | ||
| Name: "bridge", | ||
| Aliases: pq.StringArray{"winner-alias", "loser-alias"}, | ||
| UserType: "user", | ||
| ScraperID: scraperModel.ID, | ||
| CreatedAt: now, | ||
| UpdatedAt: lo.ToPtr(now), | ||
| } | ||
| Expect(tx.Table(tempTable).Create(&bridge).Error).NotTo(HaveOccurred()) | ||
|
|
||
| var merges []struct { | ||
| LoserID uuid.UUID `gorm:"column:loser_id"` | ||
| WinnerID uuid.UUID `gorm:"column:winner_id"` | ||
| } | ||
| Expect(tx.Raw("SELECT * FROM merge_and_upsert_external_users(?)", tempTable).Scan(&merges).Error).NotTo(HaveOccurred()) | ||
| Expect(tx.Commit().Error).NotTo(HaveOccurred()) |
Contributor
There was a problem hiding this comment.
Register cleanup before the setup can fail.
The first cleanup hook is added only after all inserts succeed. Any Expect failure above Line 104 leaks fixed IDs like winnerID, loserID, winner-access, and loser-access into the suite DB, and a failure after Begin() leaves the transaction open. Use DeferCleanup immediately after each successful create and add a rollback cleanup right after Begin().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scrapers/external_entity_merge_test.go` around lines 104 - 134, The current
teardown defer runs only after setup completes, which risks leaking created
records and leaving transactions open on failure; fix by registering cleanup
immediately after each successful create using the test framework's DeferCleanup
for the created records (e.g., the created ConfigItem, scraperModel,
ExternalUser entries like bridge with ID bridgeID and any access logs using
DefaultContext.DB().Create) and, right after calling tx :=
DefaultContext.DB().Begin(), register a rollback cleanup that calls
tx.Rollback() (or tx.RollbackUnlessCommitted) to ensure the transaction is
closed on test failure; keep the existing final deletions as an extra cleanup
but move/augment record-specific cleanup to immediately follow each successful
insert (and register tempTable cleanup if applicable) so no resources leak on
early failures.
Comment on lines
+205
to
+209
| row := dutymodels.ConfigAccess{ | ||
| ID: ca.ID, | ||
| ConfigID: configIDByExternalID[ca.ConfigExternalID], | ||
| ScraperID: &scraperModel.ID, | ||
| CreatedAt: now.Add(time.Duration(ca.CreatedAtOffsetMins) * time.Minute), |
Contributor
There was a problem hiding this comment.
Fail fast on unknown config_external_id values.
configIDByExternalID[ca.ConfigExternalID] falls back to the zero UUID. A bad fixture reference then shows up as a generic insert/FK failure instead of pointing at the invalid config_external_id.
Suggested guard
for _, ca := range fixture.PrePopulate.ConfigAccess {
+ configID, ok := configIDByExternalID[ca.ConfigExternalID]
+ Expect(ok).To(BeTrue(), "missing pre-populated config external_id %s", ca.ConfigExternalID)
row := dutymodels.ConfigAccess{
ID: ca.ID,
- ConfigID: configIDByExternalID[ca.ConfigExternalID],
+ ConfigID: configID,
ScraperID: &scraperModel.ID,
CreatedAt: now.Add(time.Duration(ca.CreatedAtOffsetMins) * time.Minute),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| row := dutymodels.ConfigAccess{ | |
| ID: ca.ID, | |
| ConfigID: configIDByExternalID[ca.ConfigExternalID], | |
| ScraperID: &scraperModel.ID, | |
| CreatedAt: now.Add(time.Duration(ca.CreatedAtOffsetMins) * time.Minute), | |
| for _, ca := range fixture.PrePopulate.ConfigAccess { | |
| configID, ok := configIDByExternalID[ca.ConfigExternalID] | |
| Expect(ok).To(BeTrue(), "missing pre-populated config external_id %s", ca.ConfigExternalID) | |
| row := dutymodels.ConfigAccess{ | |
| ID: ca.ID, | |
| ConfigID: configID, | |
| ScraperID: &scraperModel.ID, | |
| CreatedAt: now.Add(time.Duration(ca.CreatedAtOffsetMins) * time.Minute), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scrapers/extract_e2e_test.go` around lines 205 - 209, The test currently uses
configIDByExternalID[ca.ConfigExternalID] which returns a zero UUID for unknown
keys; instead, explicitly look up the key (e.g. id, ok :=
configIDByExternalID[ca.ConfigExternalID]) before constructing
dutymodels.ConfigAccess and fail fast if not found (t.Fatalf or return an error)
with a clear message including ca.ConfigExternalID so a bad fixture reference is
reported directly rather than causing a generic FK insert failure.
Comment on lines
+232
to
+243
| for _, log := range fixture.PrePopulate.ConfigAccessLogs { | ||
| id, ok := userIDByAlias[log.ExternalUserAliases[0]] | ||
| Expect(ok).To(BeTrue(), "missing pre-populated external user alias %s", log.ExternalUserAliases[0]) | ||
| count := log.Count | ||
| row := dutymodels.ConfigAccessLog{ | ||
| ConfigID: configIDByExternalID[log.ConfigExternalID], | ||
| ExternalUserID: id, | ||
| ScraperID: scraperModel.ID, | ||
| CreatedAt: now.Add(time.Duration(log.CreatedAtOffsetMins) * time.Minute), | ||
| Count: &count, | ||
| Properties: log.Properties, | ||
| } |
Contributor
There was a problem hiding this comment.
Validate config_access_logs inputs before indexing them.
log.ExternalUserAliases[0] will panic on an empty list, and the config lookup has the same zero-UUID fallback as above. Bad fixtures should fail with an assertion, not an index panic or opaque FK error.
Suggested guard
for _, log := range fixture.PrePopulate.ConfigAccessLogs {
- id, ok := userIDByAlias[log.ExternalUserAliases[0]]
- Expect(ok).To(BeTrue(), "missing pre-populated external user alias %s", log.ExternalUserAliases[0])
+ Expect(log.ExternalUserAliases).ToNot(BeEmpty(), "config_access_log requires at least one external_user_alias")
+ alias := log.ExternalUserAliases[0]
+ configID, ok := configIDByExternalID[log.ConfigExternalID]
+ Expect(ok).To(BeTrue(), "missing pre-populated config external_id %s", log.ConfigExternalID)
+ id, ok := userIDByAlias[alias]
+ Expect(ok).To(BeTrue(), "missing pre-populated external user alias %s", alias)
count := log.Count
row := dutymodels.ConfigAccessLog{
- ConfigID: configIDByExternalID[log.ConfigExternalID],
+ ConfigID: configID,
ExternalUserID: id,
ScraperID: scraperModel.ID,
CreatedAt: now.Add(time.Duration(log.CreatedAtOffsetMins) * time.Minute),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, log := range fixture.PrePopulate.ConfigAccessLogs { | |
| id, ok := userIDByAlias[log.ExternalUserAliases[0]] | |
| Expect(ok).To(BeTrue(), "missing pre-populated external user alias %s", log.ExternalUserAliases[0]) | |
| count := log.Count | |
| row := dutymodels.ConfigAccessLog{ | |
| ConfigID: configIDByExternalID[log.ConfigExternalID], | |
| ExternalUserID: id, | |
| ScraperID: scraperModel.ID, | |
| CreatedAt: now.Add(time.Duration(log.CreatedAtOffsetMins) * time.Minute), | |
| Count: &count, | |
| Properties: log.Properties, | |
| } | |
| for _, log := range fixture.PrePopulate.ConfigAccessLogs { | |
| Expect(log.ExternalUserAliases).ToNot(BeEmpty(), "config_access_log requires at least one external_user_alias") | |
| alias := log.ExternalUserAliases[0] | |
| configID, ok := configIDByExternalID[log.ConfigExternalID] | |
| Expect(ok).To(BeTrue(), "missing pre-populated config external_id %s", log.ConfigExternalID) | |
| id, ok := userIDByAlias[alias] | |
| Expect(ok).To(BeTrue(), "missing pre-populated external user alias %s", alias) | |
| count := log.Count | |
| row := dutymodels.ConfigAccessLog{ | |
| ConfigID: configID, | |
| ExternalUserID: id, | |
| ScraperID: scraperModel.ID, | |
| CreatedAt: now.Add(time.Duration(log.CreatedAtOffsetMins) * time.Minute), | |
| Count: &count, | |
| Properties: log.Properties, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scrapers/extract_e2e_test.go` around lines 232 - 243, The loop over
fixture.PrePopulate.ConfigAccessLogs must validate inputs before using them:
ensure each log has at least one ExternalUserAliases entry (check
len(log.ExternalUserAliases) > 0) and assert the alias maps via userIDByAlias,
and assert the ConfigExternalID maps via configIDByExternalID (and is not the
zero/empty UUID) instead of indexing or falling back; use Expect(...) to fail
the test with clear messages referencing fixture.PrePopulate.ConfigAccessLogs,
log.ExternalUserAliases, userIDByAlias, and configIDByExternalID so bad fixtures
produce assertions rather than panics or FK errors.
2a2437e to
8733778
Compare
Contributor
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 `@build/Dockerfile`:
- Line 5: Replace lowercase "as" with uppercase "AS" in the Dockerfile FROM
statements to satisfy the FromAsCasing lint rule; specifically update lines like
"FROM rust-builder-base as rust-builder" (and the other occurrences at the
second and third build stage lines) to use "AS" (e.g., "FROM rust-builder-base
AS rust-builder") so all build stage aliases are consistently uppercased.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
build/Dockerfile (1)
5-5:⚠️ Potential issue | 🟡 MinorUse uppercase
ASinFROM ... AS ...stage aliases.Line 5, Line 11, and Line 20 still use lowercase
as, which keeps the Docker lint warning active (FromAsCasing).Proposed fix
-FROM rust-builder-base as rust-builder +FROM rust-builder-base AS rust-builder @@ -FROM golang:1.25.8-bookworm@sha256:7af46e70d2017aef0b4ce2422afbcf39af0511a61993103e948b61011233ec42 as builder-base +FROM golang:1.25.8-bookworm@sha256:7af46e70d2017aef0b4ce2422afbcf39af0511a61993103e948b61011233ec42 AS builder-base @@ -FROM builder-base as builder +FROM builder-base AS builder#!/bin/bash # Verify remaining lowercase stage aliases in Dockerfile. rg -nP '^\s*FROM\s+.+\sas\s+\S+' build/Dockerfile # Expected after fix: no matches.Also applies to: 11-11, 20-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/Dockerfile` at line 5, Update the Dockerfile FROM stage alias keywords to uppercase AS: locate all FROM directives that use lowercase "as" (e.g., "FROM rust-builder-base as rust-builder" and the other stage aliases in the same file) and replace "as" with "AS" so each stage reads "FROM <image> AS <stage>". Ensure every stage alias (rust-builder and the other named stages) uses uppercase AS to silence the FromAsCasing lint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/Dockerfile`:
- Line 24: The COPY instruction 'COPY --from=rust-builder
/app/external/diffgen/target ./external/diffgen/target ' contains a trailing
non-breaking space character in the destination path; open the Dockerfile,
locate the COPY line (the COPY --from=rust-builder ... ./external/diffgen/target
entry) and remove the trailing non-breaking space so the destination is exactly
"./external/diffgen/target" (no hidden characters), then save and re-run the
build to verify artifacts are placed correctly.
---
Duplicate comments:
In `@build/Dockerfile`:
- Line 5: Update the Dockerfile FROM stage alias keywords to uppercase AS:
locate all FROM directives that use lowercase "as" (e.g., "FROM
rust-builder-base as rust-builder" and the other stage aliases in the same file)
and replace "as" with "AS" so each stage reads "FROM <image> AS <stage>". Ensure
every stage alias (rust-builder and the other named stages) uses uppercase AS to
silence the FromAsCasing lint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
moshloop
force-pushed
the
pr/fix-external-merge-error
branch
from
d2082cf to
dbed79f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build/Dockerfile (1)
1-8: The Rust cache split does not optimize reuse yet.Lines 2 and 8 copy the full
external/diffgentree, with the full source appearing aftercargo fetchin the base stage. Any source edit under that crate will invalidate the dependency-fetch layer, so the separaterust-builder-basestage provides minimal cache benefit. Since this crate has no path dependencies, copy only the manifests (Cargo.tomlandCargo.lock) in the fetch stage and move the full source copy torust-builder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/Dockerfile` around lines 1 - 8, The current multi-stage Dockerfile copies the full external/diffgen source in the rust-builder-base stage which invalidates the cargo fetch cache on any source change; update the rust-builder-base stage to copy only the crate manifests (external/diffgen/Cargo.toml and external/diffgen/Cargo.lock) before running cargo fetch, and move the full source copy (COPY external/diffgen /app/external/diffgen) into the rust-builder stage (where WORKDIR /app and subsequent build steps run) so dependency fetching is cached as intended for the crate referenced by external/diffgen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/Dockerfile`:
- Around line 36-39: The group/user creation is not idempotent: instead of
blindly attempting groupmod/groupadd and usermod/useradd with hard-coded
1000:1000, make the block idempotent by first resolving the actual catalog
UID/GID (e.g., via getent) or detecting an existing ubuntu account, then: if the
catalog group exists reuse its GID, else if ubuntu exists rename it to catalog
but force a specific GID if you require 1000, otherwise create catalog with the
chosen GID using groupadd; likewise for the user: if catalog user exists reuse
its UID, else rename ubuntu to catalog with explicit UID or create catalog with
the chosen UID using useradd/usermod while preserving home (-d/-m); finally
chown /opt/database and /app using the resolved UID:GID (not the literal
1000:1000) so USER catalog:catalog will own those paths reliably.
---
Nitpick comments:
In `@build/Dockerfile`:
- Around line 1-8: The current multi-stage Dockerfile copies the full
external/diffgen source in the rust-builder-base stage which invalidates the
cargo fetch cache on any source change; update the rust-builder-base stage to
copy only the crate manifests (external/diffgen/Cargo.toml and
external/diffgen/Cargo.lock) before running cargo fetch, and move the full
source copy (COPY external/diffgen /app/external/diffgen) into the rust-builder
stage (where WORKDIR /app and subsequent build steps run) so dependency fetching
is cached as intended for the crate referenced by external/diffgen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+36
to
39
| RUN mkdir -p /opt/database && \ | ||
| (groupmod -n catalog ubuntu 2>/dev/null || groupadd --gid 1000 catalog) && \ | ||
| (usermod -l catalog -d /var/lib/catalog -m ubuntu 2>/dev/null || useradd catalog --uid 1000 -g catalog -m -d /var/lib/catalog) && \ | ||
| chown -R 1000:1000 /opt/database && chown -R 1000:1000 /app |
Contributor
There was a problem hiding this comment.
This user/group block is still not idempotent.
Lines 37-38 only handle “rename ubuntu” or “create catalog”. If the base image already ships catalog, the fallback groupadd/useradd path fails; and on the rename path, Line 39 still hard-codes 1000:1000, which can diverge from the renamed account’s real UID/GID. That makes the build brittle and can leave USER catalog:catalog without ownership of /app or /opt/database. If you rely on a fixed 1000:1000 identity for bind mounts, the rename path also needs explicit -u/-g 1000.
🐛 Proposed fix
RUN mkdir -p /opt/database && \
- (groupmod -n catalog ubuntu 2>/dev/null || groupadd --gid 1000 catalog) && \
- (usermod -l catalog -d /var/lib/catalog -m ubuntu 2>/dev/null || useradd catalog --uid 1000 -g catalog -m -d /var/lib/catalog) && \
- chown -R 1000:1000 /opt/database && chown -R 1000:1000 /app
+ if getent group catalog >/dev/null; then :; \
+ elif getent group ubuntu >/dev/null; then groupmod -n catalog ubuntu; \
+ else groupadd --gid 1000 catalog; fi && \
+ if id -u catalog >/dev/null 2>&1; then :; \
+ elif id -u ubuntu >/dev/null 2>&1; then usermod -l catalog -d /var/lib/catalog -m ubuntu; \
+ else useradd catalog --uid 1000 -g catalog -m -d /var/lib/catalog; fi && \
+ chown -R catalog:catalog /opt/database /app📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RUN mkdir -p /opt/database && \ | |
| (groupmod -n catalog ubuntu 2>/dev/null || groupadd --gid 1000 catalog) && \ | |
| (usermod -l catalog -d /var/lib/catalog -m ubuntu 2>/dev/null || useradd catalog --uid 1000 -g catalog -m -d /var/lib/catalog) && \ | |
| chown -R 1000:1000 /opt/database && chown -R 1000:1000 /app | |
| RUN mkdir -p /opt/database && \ | |
| if getent group catalog >/dev/null; then :; \ | |
| elif getent group ubuntu >/dev/null; then groupmod -n catalog ubuntu; \ | |
| else groupadd --gid 1000 catalog; fi && \ | |
| if id -u catalog >/dev/null 2>&1; then :; \ | |
| elif id -u ubuntu >/dev/null 2>&1; then usermod -l catalog -d /var/lib/catalog -m ubuntu; \ | |
| else useradd catalog --uid 1000 -g catalog -m -d /var/lib/catalog; fi && \ | |
| chown -R catalog:catalog /opt/database /app |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build/Dockerfile` around lines 36 - 39, The group/user creation is not
idempotent: instead of blindly attempting groupmod/groupadd and usermod/useradd
with hard-coded 1000:1000, make the block idempotent by first resolving the
actual catalog UID/GID (e.g., via getent) or detecting an existing ubuntu
account, then: if the catalog group exists reuse its GID, else if ubuntu exists
rename it to catalog but force a specific GID if you require 1000, otherwise
create catalog with the chosen GID using groupadd; likewise for the user: if
catalog user exists reuse its UID, else rename ubuntu to catalog with explicit
UID or create catalog with the chosen UID using useradd/usermod while preserving
home (-d/-m); finally chown /opt/database and /app using the resolved UID:GID
(not the literal 1000:1000) so USER catalog:catalog will own those paths
reliably.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit