Skip to content

refactor(render): migrate phases 1 and 3 to parmap.Map#173

Merged
dmcilvaney merged 1 commit into
microsoft:mainfrom
dmcilvaney:damcilva/refactor_render
May 13, 2026
Merged

refactor(render): migrate phases 1 and 3 to parmap.Map#173
dmcilvaney merged 1 commit into
microsoft:mainfrom
dmcilvaney:damcilva/refactor_render

Conversation

@dmcilvaney
Copy link
Copy Markdown
Contributor

Replaces the hand-rolled semaphore + waitgroup + resultsChan
pattern in both parallelPrepare (phase 1) and
parallelFinish (phase 3) with calls to parmap.Map.

  • Drops the prepWithSemaphore indirection in phase 1; the
    worker is now just prepareOneComponent (validate + prepare).
  • Drops the semaphore parameter from finishOneComponent and
    the trailing manual fanout loop in phase 3.
  • Removes the prepResult.index field — parmap returns results
    in input order, so callers index into them directly.
  • Cancellation-before-start (worker never invoked) surfaces via
    Result.Cancelled and is mapped to renderStatusCancelled
    at the loop callsites, matching the pre-refactor behaviour.
  • Drops the unused sync import; adds context and parmap.

No behaviour change. mage unit + mage scenario pass.

Copilot AI review requested due to automatic review settings May 10, 2026 22:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the component render/update parallel phases to use a new bounded parallel map helper (internal/utils/parmap.Map) instead of hand-rolled semaphore + goroutine + channel patterns.

Changes:

  • Added internal/utils/parmap package (errgroup + SetLimit) with per-item ordered results and a “cancelled-before-start” signal.
  • Migrated component render phase 1 (prepare) and phase 3 (finish) to parmap.Map.
  • Migrated component update parallel source-identity resolution to parmap.Map, including a synchronous pre-filter step for “fresh” components.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/utils/parmap/parmap.go New bounded parallel map helper used by render/update refactors
internal/utils/parmap/parmap_test.go Unit tests for parmap.Map behavior (ordering, limits, cancellation, progress)
internal/app/azldev/cmds/component/render.go Refactors render phases 1 & 3 to use parmap.Map
internal/app/azldev/cmds/component/update.go Refactors parallel identity resolution to use parmap.Map + pre-filter
go.mod Adds golang.org/x/sync dependency
go.sum Adds checksums for golang.org/x/sync

Comment thread internal/utils/parmap/parmap.go Outdated
Comment thread internal/app/azldev/cmds/component/render.go
Comment thread internal/app/azldev/cmds/component/render.go
Comment thread internal/app/azldev/cmds/component/update.go
@dmcilvaney dmcilvaney force-pushed the damcilva/refactor_render branch from d522650 to 1c52356 Compare May 10, 2026 22:26
Copilot AI review requested due to automatic review settings May 10, 2026 22:36
@dmcilvaney dmcilvaney force-pushed the damcilva/refactor_render branch from 1c52356 to 8e7782f Compare May 10, 2026 22:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Comment thread internal/utils/parmap/parmap.go
Comment thread internal/app/azldev/cmds/component/render.go
Comment thread internal/utils/parmap/parmap.go
@dmcilvaney
Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

Replaces the hand-rolled semaphore + waitgroup + resultsChan
pattern in both `parallelPrepare` (phase 1) and
`parallelFinish` (phase 3) with calls to `parmap.Map`.

- Drops the `prepWithSemaphore` indirection in phase 1; the
  worker is now just `prepareOneComponent` (validate + prepare).
- Drops the semaphore parameter from `finishOneComponent` and
  the trailing manual fanout loop in phase 3.
- Removes the `prepResult.index` field — parmap returns results
  in input order, so callers index into them directly.
- Cancellation-before-start (worker never invoked) surfaces via
  `Result.Cancelled` and is mapped to `renderStatusCancelled`
  at the loop callsites, matching the pre-refactor behaviour.
- Drops the unused `sync` import; adds `context` and `parmap`.

No behaviour change. `mage unit` + `mage scenario` pass.
@dmcilvaney dmcilvaney force-pushed the damcilva/refactor_render branch from 8e7782f to a149660 Compare May 12, 2026 05:06
@dmcilvaney dmcilvaney marked this pull request as ready for review May 12, 2026 05:06
Copilot AI review requested due to automatic review settings May 12, 2026 05:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@dmcilvaney dmcilvaney merged commit f936cec into microsoft:main May 13, 2026
19 checks passed
@dmcilvaney dmcilvaney deleted the damcilva/refactor_render branch May 13, 2026 23:12
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