Give transfer/sync-issues independent source/target concurrency and timeout - #553
Conversation
…ncy and timeout (#528) The network path from the migration host to the source SonarQube Server can differ significantly from the path to the target SonarQube Cloud, so a single shared concurrency/timeout doesn't fit both sides. extract and migrate already resolve concurrency/timeout independently per side via the unified source/target config-file shape (#266) and use fully separate semaphores and HTTP clients. transfer and sync-issues had the same bug: both load the config file through both extract's and migrate's own loaders (each already resolving its side correctly), but then collapsed the two independently-resolved values into one shared field — always discarding target.timeout in favor of source.timeout, and arbitrarily preferring one side's concurrency when both were set. Split transferConfig/syncIssuesConfig's concurrency/timeout into source*/target* pairs, assign the two loaders' resolved values straight through instead of collapsing them, and route each phase's API config to its own pair. --concurrency/--timeout on the CLI now set both sides at once (per the issue: the config file is the only way to differ them for these two commands, since they talk to both sides in one invocation).
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
SonarCloud's PR scan failed the quality gate on new_duplicated_lines_density (39.7%, threshold 3%): the "--concurrency/--timeout set both source and target" block was copy-pasted near-verbatim between transfer.go and sync_issues.go. Extracted a shared applyFlagIntBothSides helper (next to the existing applyFlagString/applyFlagInt/applyFlagBool) and use it from both commands instead.
… clarify flag help text Co-authored-by: Olivier K <161828284+okorach-sonar@users.noreply.github.com>
Gitar's auto-applied fix for "preserve per-side concurrency/timeout with single-side fallback" pasted an identical 12-line block into both transfer.go and sync_issues.go, spiking new_duplicated_lines_density to 53.3% (threshold 3%). Extracted the bidirectional fallback into a shared fallbackToOtherSide helper next to applyFlagIntBothSides, used by both loadTransferFileDefaults and loadSyncIssuesFileDefaults. Also brought sync-issues's --concurrency/--timeout flag help text in line with transfer's clarified wording, and added a regression test per command for the single-side-fallback behavior itself.
| f.Int(flagConcurrency, 0, "Max concurrent requests, applied to both source and target (default: 25). Use source.concurrency / target.concurrency in the config file to set them independently.") | ||
| f.Int(flagTimeout, 0, "HTTP request timeout in seconds, applied to both source and target (default: 60). Use source.timeout / target.timeout in the config file to set them independently.") |
There was a problem hiding this comment.
⚠️ Quality: Cross-side concurrency/timeout fill contradicts the docs it points to
The new fallbackToOtherSide makes a value set on one side leak to the other: a transfer config with only target.timeout: 30 (no top-level, nothing under source) now applies 30 to the extract phase too, and only source.concurrency: 50 now drives the SonarQube Cloud phase at 50 instead of the documented default. That directly contradicts docs/ADVANCED-CONFIG.md, which states source.concurrency/source.timeout override "for extract calls" and target.* "for migrate / reset calls", and contradicts the help text added in this same commit ("Use source.concurrency / target.concurrency in the config file to set them independently") — with the fallback, one side can never be set independently while the other keeps its default. docs/TRANSFER.md:222-223 and docs/SYNC-ISSUES.md:117-118 also still document only a single top-level concurrency/timeout field for these two commands, so the per-side fields the new help text tells operators to use are undocumented there. Update the docs (and the flag help) to state the cross-fill rule explicitly, or drop the fallback so an unset side uses the package default.
Document the cross-side fill in docs/ADVANCED-CONFIG.md (mirror the wording in the two target. rows) and add source./target. concurrency+timeout rows to docs/TRANSFER.md and docs/SYNC-ISSUES.md.:*
| `source.concurrency` | `--concurrency` | top-level | No | Override top-level concurrency for extract calls. On `transfer`/`sync-issues`, if only one side sets it, the other side borrows that value (#528). |
| `source.timeout` | `--timeout` | top-level | No | Override top-level timeout for extract calls. Same single-side borrowing rule as above on `transfer`/`sync-issues`. |
Was this helpful? React with 👍 / 👎
| func fallbackToOtherSide(a, b *int) { | ||
| if *a == 0 { | ||
| *a = *b | ||
| } | ||
| if *b == 0 { | ||
| *b = *a | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Quality: New fallback tests cover only one of the helper's two branches
Both new tests set concurrency/timeout only under target and assert only cfg.sourceConcurrency/cfg.sourceTimeout, so the if *b == 0 { *b = *a } branch of fallbackToOtherSide (source-only config → target borrows) is never executed, and neither test asserts that the explicitly-set side survives the call (a fix that overwrote the set side with the zero one would still pass). Add the mirrored source-only case and assert both sides in each test.
Assert both sides and add the mirrored source-only case so the second branch of fallbackToOtherSide is exercised.:
if cfg.sourceConcurrency != 5 {
t.Errorf("sourceConcurrency: got %d, want 5 (borrowed from target)", cfg.sourceConcurrency)
}
if cfg.targetConcurrency != 5 {
t.Errorf("targetConcurrency: got %d, want 5 (explicitly set)", cfg.targetConcurrency)
}
if cfg.sourceTimeout != 30 {
t.Errorf("sourceTimeout: got %d, want 30 (borrowed from target)", cfg.sourceTimeout)
}
if cfg.targetTimeout != 30 {
t.Errorf("targetTimeout: got %d, want 30 (explicitly set)", cfg.targetTimeout)
}
// plus a mirrored sub-test with concurrency/timeout only under "source",
// asserting target borrows them.
Was this helpful? React with 👍 / 👎
…function new_duplicated_lines_density was still at 32.9% after extracting fallbackToOtherSide: the 6-line "resolve source/target concurrency and timeout" sequence around each fallbackToOtherSide call was still copy-pasted identically into both loadTransferFileDefaults and loadSyncIssuesFileDefaults. Folded the whole sequence into a single resolveSourceTargetRates(extractCfg, migrateCfg) function next to fallbackToOtherSide, called with one line from each command.
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
4 participants
Summary
Fixes #528 —
transferandsync-issuesnow resolveconcurrency/timeoutindependently for source (SonarQube Server) and target (SonarQube Cloud), instead of silently collapsing the two into one shared value.extractandmigratealready resolved these correctly per side (issue #266's unified config shape), each with its own semaphore and HTTP client. The bug was confined totransfer/sync-issues, which load the config file through bothextract.LoadExtractConfigFile(source-resolved) andmigrate.LoadMigrateConfigFile(target-resolved) but then collapsed the two already-correct values into one field:So
source.timeout: 30, target.timeout: 90in a config file used to apply 30 to both phases — permanently discarding the target value — even though the documented example config (examples/config.unified.example.json) andtransfer's own--helptext have shown independent per-side values since #266.transferConfig/syncIssuesConfig'sconcurrency/timeoutfields intosource*/target*pairs.loadTransferFileDefaults/loadSyncIssuesFileDefaults: assign each loader's resolved value straight through instead of collapsing.--concurrency/--timeouton the CLI now set both sides at once, matching the issue's spec that the config file is the only way to differ them for these two commands.extract.RunExtract,migrate.RunMigrate/RunSyncIssues) now gets its own side's values.Test plan
go build ./...,go vet ./...cleango test ./... -count=1— full suite greenTestResolveTransferConfig_UnifiedConfigShape/TestResolveSyncIssuesConfig_UnifiedConfigShapefor the renamed fieldsTestResolveTransferConfig_CLIOverridesConfig/TestResolveSyncIssuesConfig_CLIOverridesConfigto assert--concurrency/--timeoutset both source and targetTestResolveTransferConfig_SourceAndTargetConcurrencyTimeoutDifferandTestResolveSyncIssuesConfig_SourceAndTargetConcurrencyTimeoutDiffer— regression tests proving distinct per-side config-file values resolve correctly (the scenario the old tests never covered)transfer --helpoutput — flag descriptions still read correctly