Conversation
Add migration script to convert project settings from old format (string arrays for writers/auditors) to new UserInfo format with name, username, email, and avatar fields. The script: - Connects to NATS and checks project-settings KV store - Detects format and prompts for user details if migration needed - Updates NATS with migrated data and sends indexer sync message - Includes Makefile and documentation for easy usage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jordan Evans <jevans@linuxfoundation.org>
WalkthroughAdds a new Go-based migration tool under scripts/migrate-project-settings with a Makefile and README. The tool connects to NATS JetStream, reads project settings from a KV store, detects old vs. new format, interactively collects missing user details, writes back the new format, and publishes an indexer sync message. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as migrate-project-settings (CLI)
participant N as NATS
participant JS as JetStream
participant KV as Project Settings KV
participant IDX as Indexer
U->>CLI: run with PROJECT_UID
CLI->>N: Connect (NATS_URL)
N-->>CLI: Connection
CLI->>JS: Create context
CLI->>KV: Get(project_uid)
KV-->>CLI: JSON entry
alt Entry in new format
CLI-->>U: Prints "already in new format"
CLI-->>U: Exit
else Legacy format
CLI->>U: Prompt for auditors/writers details
U-->>CLI: Provide UserInfo inputs
CLI->>KV: Put(project_uid, new JSON)
KV-->>CLI: Ack
CLI->>IDX: Publish index sync (ActionUpdated, tags)
IDX-->>CLI: Ack
CLI-->>U: Complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a migration script to convert project settings from an old format (using string arrays for writers/auditors) to a new UserInfo format with structured user data including name, username, email, and avatar fields.
- Implements migration logic with interactive prompts for user details
- Adds NATS connectivity to read/write project settings and send indexer sync messages
- Provides comprehensive documentation and build automation via Makefile
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/migrate-project-settings/main.go | Core migration script with NATS integration and user input handling |
| scripts/migrate-project-settings/README.md | Documentation explaining usage, environment variables, and example workflow |
| scripts/migrate-project-settings/Makefile | Build automation with targets for running, building, and testing the script |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if len(newSettings.Auditors) > 0 && newSettings.Auditors[0].Name != "" { | ||
| fmt.Printf("Project %s is already in new format\n", projectUID) | ||
| return | ||
| } | ||
| if len(newSettings.Writers) > 0 && newSettings.Writers[0].Name != "" { | ||
| fmt.Printf("Project %s is already in new format\n", projectUID) | ||
| return | ||
| } |
There was a problem hiding this comment.
The format detection logic is flawed. It only checks the first element of auditors/writers arrays, but an array could have empty UserInfo structs in the first position while containing valid data later. This could cause the script to incorrectly identify new format data as old format and attempt unnecessary migration.
| // Check if it's already in new format (has UserInfo structs) | ||
| if len(newSettings.Auditors) > 0 || len(newSettings.Writers) > 0 { | ||
| // Check if first auditor/writer has the UserInfo structure | ||
| if len(newSettings.Auditors) > 0 && newSettings.Auditors[0].Name != "" { | ||
| fmt.Printf("Project %s is already in new format\n", projectUID) | ||
| return | ||
| } | ||
| if len(newSettings.Writers) > 0 && newSettings.Writers[0].Name != "" { | ||
| fmt.Printf("Project %s is already in new format\n", projectUID) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
The migration detection logic has a critical flaw. If the JSON unmarshaling succeeds into the new format but the arrays are empty, the script will fall through to the old format processing, potentially causing data corruption or errors when trying to process UserInfo objects as strings.
| // Check if it's already in new format (has UserInfo structs) | |
| if len(newSettings.Auditors) > 0 || len(newSettings.Writers) > 0 { | |
| // Check if first auditor/writer has the UserInfo structure | |
| if len(newSettings.Auditors) > 0 && newSettings.Auditors[0].Name != "" { | |
| fmt.Printf("Project %s is already in new format\n", projectUID) | |
| return | |
| } | |
| if len(newSettings.Writers) > 0 && newSettings.Writers[0].Name != "" { | |
| fmt.Printf("Project %s is already in new format\n", projectUID) | |
| return | |
| } | |
| } | |
| // If unmarshaling into new format succeeds, treat as new format regardless of array contents | |
| fmt.Printf("Project %s is already in new format\n", projectUID) | |
| return |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
scripts/migrate-project-settings/main.go (3)
49-55: Harden NATS connection (timeouts, retries, client name)Improve resiliency for ops usage.
- nc, err := nats.Connect(natsURL) + nc, err := nats.Connect( + natsURL, + nats.Name("lfx-v2 project-settings migrator"), + nats.Timeout(10*time.Second), + nats.RetryOnFailedConnect(true), + nats.MaxReconnects(5), + nats.ReconnectWait(2*time.Second), + )
118-140: Avoid duplicate prompts for the same user across writers/auditorsCache entered UserInfo by username and reuse.
- reader := bufio.NewReader(os.Stdin) + reader := bufio.NewReader(os.Stdin) + userCache := map[string]models.UserInfo{} @@ - for _, auditorStr := range oldSettings.Auditors { + for _, auditorStr := range oldSettings.Auditors { if auditorStr == "" { continue } fmt.Printf("\nMigrating auditor: %s\n", auditorStr) - userInfo := getUserInfo(reader, auditorStr) + userInfo, ok := userCache[auditorStr] + if !ok { + userInfo = getUserInfo(reader, auditorStr) + userCache[auditorStr] = userInfo + } newSettings.Auditors = append(newSettings.Auditors, userInfo) } @@ - for _, writerStr := range oldSettings.Writers { + for _, writerStr := range oldSettings.Writers { if writerStr == "" { continue } fmt.Printf("\nMigrating writer: %s\n", writerStr) - userInfo := getUserInfo(reader, writerStr) + userInfo, ok := userCache[writerStr] + if !ok { + userInfo = getUserInfo(reader, writerStr) + userCache[writerStr] = userInfo + } newSettings.Writers = append(newSettings.Writers, userInfo) }
162-176: Bound publish with a timeoutPrevent hanging publish calls.
- // Send indexer sync message - ctx := context.Background() + // Send indexer sync message + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel()scripts/migrate-project-settings/Makefile (1)
4-4: Address checkmake warnings; simplify run; add all targetAdd “all”, factor PROJECT_UID check, and use module‑root patterns.
-.PHONY: help run build clean test +.PHONY: help run build clean test all check-project-uid @@ -run: ## Run the migration script (requires PROJECT_UID) - @if [ -z "$(PROJECT_UID)" ]; then \ - echo "Usage: make run PROJECT_UID=<project-uid>"; \ - echo "Example: make run PROJECT_UID=7cad5a8d-19d0-41a4-81a6-043453daf9ee"; \ - exit 1; \ - fi - go run main.go $(PROJECT_UID) +run: check-project-uid ## Run the migration script (requires PROJECT_UID) + go run . $(PROJECT_UID) + +check-project-uid: + @test -n "$(PROJECT_UID)" || { echo "Usage: make run PROJECT_UID=<project-uid>"; exit 1; } @@ -build: ## Build the migration script - go build -o bin/migrate-project-settings main.go +build: ## Build the migration script + go build -o bin/migrate-project-settings . @@ .DEFAULT_GOAL := help +all: build ## Default aggregate targetAlso applies to: 9-16, 17-26
scripts/migrate-project-settings/README.md (1)
7-10: Polish README: add Makefile usage and code fence languageAdd make run example and specify language for the prompts block.
@@ ```bash cd scripts/migrate-project-settings go run main.go <project-uid> +make run PROJECT_UID=<project-uid>@@
-# Run migration for a specific project
-go run main.go 7cad5a8d-19d0-41a4-81a6-043453daf9ee
+# Run migration for a specific project
+go run main.go 7cad5a8d-19d0-41a4-81a6-043453daf9ee
+# or using Makefile
+make run PROJECT_UID=7cad5a8d-19d0-41a4-81a6-043453daf9ee
@@
-+text
Migrating writer: johndoe
Enter details for user 'johndoe':
Name: John Doe
Username [johndoe]:
Email: john.doe@example.com
Avatar URL (optional): https://example.com/avatar.jpg
@@
Avatar URL (optional):Also applies to: 26-32, 36-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
scripts/migrate-project-settings/Makefile(1 hunks)scripts/migrate-project-settings/README.md(1 hunks)scripts/migrate-project-settings/main.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure code formatting and linting pass (make fmt, make lint, make check)
Files:
scripts/migrate-project-settings/main.go
🧬 Code graph analysis (1)
scripts/migrate-project-settings/main.go (2)
pkg/constants/nats.go (2)
KVStoreNameProjectSettings(12-12)IndexProjectSettingsSubject(23-23)internal/domain/models/message.go (2)
ProjectSettingsIndexerMessage(27-31)ActionUpdated(14-14)
🪛 checkmake (0.2.2)
scripts/migrate-project-settings/Makefile
[warning] 9-9: Target body for "run" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 4-4: Missing required phony target "all"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
scripts/migrate-project-settings/README.md
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: MegaLinter
- GitHub Check: Build and Test
🔇 Additional comments (1)
scripts/migrate-project-settings/main.go (1)
16-21: nats.go version is already pinned
go.mod specifies github.com/nats-io/nats.go v1.43.0.
| // Get current project settings | ||
| entry, err := kv.Get(context.Background(), projectUID) | ||
| if err != nil { | ||
| slog.Error("Failed to get project settings", "uid", projectUID, "error", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Handle missing KV keys gracefully
If the project key is absent, exit with a clear message instead of a generic error.
- entry, err := kv.Get(context.Background(), projectUID)
+ entry, err := kv.Get(context.Background(), projectUID)
if err != nil {
- slog.Error("Failed to get project settings", "uid", projectUID, "error", err)
- os.Exit(1)
+ // Treat not found as a no-op
+ if err == jetstream.ErrKeyNotFound || err == nats.ErrKeyNotFound {
+ slog.Warn("No project settings found in KV", "uid", projectUID)
+ os.Exit(0)
+ }
+ slog.Error("Failed to get project settings", "uid", projectUID, "error", err)
+ os.Exit(1)
}Note: add imports if needed.
📝 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.
| // Get current project settings | |
| entry, err := kv.Get(context.Background(), projectUID) | |
| if err != nil { | |
| slog.Error("Failed to get project settings", "uid", projectUID, "error", err) | |
| os.Exit(1) | |
| } | |
| // Get current project settings | |
| entry, err := kv.Get(context.Background(), projectUID) | |
| if err != nil { | |
| // Treat not found as a no-op | |
| if err == jetstream.ErrKeyNotFound || err == nats.ErrKeyNotFound { | |
| slog.Warn("No project settings found in KV", "uid", projectUID) | |
| os.Exit(0) | |
| } | |
| slog.Error("Failed to get project settings", "uid", projectUID, "error", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
In scripts/migrate-project-settings/main.go around lines 71 to 76, the current
kv.Get error handling treats a missing project key the same as other errors;
change it to detect a "key not found" condition (using the specific error
returned by your KV client or errors.Is/strings.Contains if client uses text
messages), and when the key is absent log a clear message like "Project settings
not found" with the uid and exit with non-zero; for other errors keep logging
the full error and exit. Add any required imports (e.g., "errors" or the KV
client's error package) to support errors.Is or the specific error type check.
| // Try to unmarshal as new format first | ||
| var newSettings models.ProjectSettings | ||
| if err := json.Unmarshal(entry.Value(), &newSettings); err == nil { | ||
| // Check if it's already in new format (has UserInfo structs) | ||
| if len(newSettings.Auditors) > 0 || len(newSettings.Writers) > 0 { | ||
| // Check if first auditor/writer has the UserInfo structure | ||
| if len(newSettings.Auditors) > 0 && newSettings.Auditors[0].Name != "" { | ||
| fmt.Printf("Project %s is already in new format\n", projectUID) | ||
| return | ||
| } | ||
| if len(newSettings.Writers) > 0 && newSettings.Writers[0].Name != "" { | ||
| fmt.Printf("Project %s is already in new format\n", projectUID) | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix new‑format detection to avoid false negatives and hard exits
If JSON is valid for models.ProjectSettings, treat it as “already migrated.” Current Name checks can fail when Name is empty but arrays are objects, leading to an old‑format unmarshal error and exit.
Apply:
- // Try to unmarshal as new format first
+ // Try to unmarshal as new format first
var newSettings models.ProjectSettings
- if err := json.Unmarshal(entry.Value(), &newSettings); err == nil {
- // Check if it's already in new format (has UserInfo structs)
- if len(newSettings.Auditors) > 0 || len(newSettings.Writers) > 0 {
- // Check if first auditor/writer has the UserInfo structure
- if len(newSettings.Auditors) > 0 && newSettings.Auditors[0].Name != "" {
- fmt.Printf("Project %s is already in new format\n", projectUID)
- return
- }
- if len(newSettings.Writers) > 0 && newSettings.Writers[0].Name != "" {
- fmt.Printf("Project %s is already in new format\n", projectUID)
- return
- }
- }
- }
+ if err := json.Unmarshal(entry.Value(), &newSettings); err == nil {
+ fmt.Printf("Project %s is already in new format\n", projectUID)
+ return
+ }🤖 Prompt for AI Agents
In scripts/migrate-project-settings/main.go around lines 78 to 93, the code
attempts to detect the "new" ProjectSettings format by unmarshaling into
models.ProjectSettings but then rejects it if Name fields are empty, causing
false negatives and exiting; change the logic so that any successful
json.Unmarshal into models.ProjectSettings is treated as "already migrated" —
remove the additional checks on Auditors/Writers' Name fields and simply log
that the project is already in the new format and return when unmarshaling
succeeds, ensuring we don't perform further old-format processing or exit
erroneously.
| // Update in NATS KV store | ||
| _, err = kv.Put(context.Background(), projectUID, newSettingsBytes) | ||
| if err != nil { | ||
| slog.Error("Failed to update project settings in KV store", "error", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use optimistic concurrency (KV.Update) to avoid overwriting concurrent edits
Replace Put with Update using the fetched entry revision.
- _, err = kv.Put(context.Background(), projectUID, newSettingsBytes)
+ _, err = kv.Update(context.Background(), projectUID, newSettingsBytes, entry.Revision())
if err != nil {
slog.Error("Failed to update project settings in KV store", "error", err)
os.Exit(1)
}📝 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.
| // Update in NATS KV store | |
| _, err = kv.Put(context.Background(), projectUID, newSettingsBytes) | |
| if err != nil { | |
| slog.Error("Failed to update project settings in KV store", "error", err) | |
| os.Exit(1) | |
| } | |
| // Update in NATS KV store | |
| _, err = kv.Update(context.Background(), projectUID, newSettingsBytes, entry.Revision()) | |
| if err != nil { | |
| slog.Error("Failed to update project settings in KV store", "error", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
scripts/migrate-project-settings/main.go around lines 153-158: the code
currently calls kv.Put which can clobber concurrent updates; replace it with
KV.Update using the entry revision fetched from the store (call kv.Get or use
the previously fetched entry) and pass entry.Revision as the expected revision
to kv.Update, handle the possible conditional update errors (e.g., key not found
or revision mismatch) by logging and retrying or exiting as appropriate, and
ensure the context and error handling remain consistent with the surrounding
code.
Add migration script to convert project settings from old format (string arrays for writers/auditors) to new UserInfo format with name, username, email, and avatar fields. The script:
🤖 Generated with Claude Code