Skip to content

migrate reviewer to agentic platform - phase 1#141

Merged
derekmisler merged 1 commit into
docker:mainfrom
derekmisler:migrate-reviewer-to-agentic-platform---phase-1
Apr 27, 2026
Merged

migrate reviewer to agentic platform - phase 1#141
derekmisler merged 1 commit into
docker:mainfrom
derekmisler:migrate-reviewer-to-agentic-platform---phase-1

Conversation

@derekmisler
Copy link
Copy Markdown
Contributor

@derekmisler derekmisler commented Apr 21, 2026

Related Issues

Closes: https://github.com/docker/gordon/issues/416

Summary

Migrate the build system from Rollup to Vite and consolidate action artifacts into a single dist/ directory at the root level. This phase 1 migration prepares the codebase for integration with the agentic platform by simplifying the build pipeline, reducing dependencies, and centralizing built artifacts.

Changes

  • Build toolchain: Replaced Rollup with Vite 6.4.2 in package.json and removed all Rollup-related plugins (@rollup/plugin-commonjs, @rollup/plugin-json, @rollup/plugin-node-resolve, @rollup/plugin-typescript, rollup-plugin-node-externals)
  • Build output structure: Consolidated setup-credentials and signed-commit artifacts into a single dist/ root directory (dist/credentials.js and dist/signed-commit.js)
  • Action entrypoints: Updated .github/actions/setup-credentials/action.yml to reference ../../../dist/credentials.js instead of the local dist/ subdirectory
  • Workflow updates: Modified .github/workflows/release.yml, update-consumers.yml, and update-docker-agent-version.yml to:
    • Use centralized dist/ file checks instead of per-action directory checks
    • Reference $GITHUB_WORKSPACE/dist/signed-commit.js and $GITHUB_WORKSPACE/dist/credentials.js for signed commit CLI invocation
    • Updated comments to reflect the new structure ("credentials and signed-commit" instead of "setup-credentials and signed-commit")
  • Test configuration: Updated package.json test scripts to use Vitest project-based configuration (--project unit and --project integration)
  • Development dependencies: Updated biome.json to simplify the organizeImports action configuration

Test plan

  • Run pnpm build to verify Vite builds both credential and signed-commit modules into dist/
  • Run pnpm test and pnpm test:integration to verify test suite runs with new Vitest project configuration
  • Run pnpm lint and pnpm actionlint to verify code quality checks pass
  • Verify release workflow passes validation checks for the new dist/ structure on next release

@derekmisler derekmisler self-assigned this Apr 21, 2026
@derekmisler
Copy link
Copy Markdown
Contributor Author

/review

@derekmisler derekmisler force-pushed the migrate-reviewer-to-agentic-platform---phase-1 branch from 9efbf05 to ffcf021 Compare April 21, 2026 00:19
Copy link
Copy Markdown
Contributor

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

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

Assessment: 🟡 NEEDS ATTENTION

Migration: Rollup → Vite, file reorganization into src/credentials/ and src/signed-commit/

One medium-severity finding surfaced (verification was inconclusive — presenting raw finding for author review).

Comment thread vite.config.ts Outdated
@derekmisler derekmisler force-pushed the migrate-reviewer-to-agentic-platform---phase-1 branch 5 times, most recently from fa86cde to 7de6094 Compare April 23, 2026 19:58
@derekmisler
Copy link
Copy Markdown
Contributor Author

/review

Copy link
Copy Markdown
Contributor

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

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

Assessment: 🟢 APPROVE

This PR migrates the credential and signed-commit modules from action-local dist/ directories (.github/actions/*/dist/) to a centralized dist/ at the repository root, switches the build toolchain from Rollup to Vite, and consolidates the Vitest config into vite.config.ts.

Changes reviewed:

  • action.yml: path updated to ../../../dist/credentials.js ✅ correct relative depth (3 levels up from .github/actions/setup-credentials/)
  • release.yml / update-docker-agent-version.yml: path replacements only — logic unchanged ✅
  • vite.config.ts (new): auto-discovery of src/<name>/index.ts entries, correct external pattern covering both node: prefixed and bare built-in modules, inlineDynamicImports: false correctly set for multi-entry builds ✅
  • package.json: clean swap of Rollup plugins for Vite; vitest run integration correctly uses Vitest's filter argument ✅
  • tsconfig.json: updated exclude list to match new file layout ✅
  • All credential and signed-commit source files: pure renames (similarity index 100%) with no content changes — existing logic preserved ✅

No bugs found in the added code.

@derekmisler derekmisler force-pushed the migrate-reviewer-to-agentic-platform---phase-1 branch 6 times, most recently from 96b7a7d to 9d4b09a Compare April 27, 2026 15:39
Signed-off-by: Derek Misler <derek.misler@docker.com>
@derekmisler derekmisler force-pushed the migrate-reviewer-to-agentic-platform---phase-1 branch from 9d4b09a to 93b7062 Compare April 27, 2026 15:52
@derekmisler derekmisler requested a review from a team April 27, 2026 15:52
@derekmisler derekmisler marked this pull request as ready for review April 27, 2026 15:52
runs:
using: node24
main: dist/setup-credentials.js
main: ../../../dist/credentials.js
Copy link
Copy Markdown
Contributor Author

@derekmisler derekmisler Apr 27, 2026

Choose a reason for hiding this comment

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

this feels weird, sure, but once everything moves to TS, this whole action isn't needed anymore, so it's only temporary.

echo "Pass 1: staging dist/ and creating temp commit..."
if [ ! -d .github/actions/setup-credentials/dist ] || [ -z "$(ls -A .github/actions/setup-credentials/dist)" ]; then
echo "::error::setup-credentials/dist/ is missing or empty — build may have failed"
if [ ! -f "$GITHUB_WORKSPACE/dist/credentials.js" ]; then
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this line here is the primary motivation for this PR, to get all of the .js files in a single directory at the root of the repo.

Copy link
Copy Markdown
Contributor

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

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

Assessment: 🟢 APPROVE

This PR cleanly migrates the build system from Rollup to Vite and consolidates the action artifacts into a unified dist/ directory. No bugs were found in the changed code.

What was reviewed:

  • vite.config.ts: Auto-discovery of entry points via src/<name>/index.ts is correct; defineConfig from vitest/config is the right import for a combined Vite+Vitest config; external modules cover both node:-prefixed and bare builtin names correctly; inlineDynamicImports: false with multiple entries is safe since the release workflow uses find dist/ -type f which includes any generated chunks.
  • action.yml: Path ../../../dist/credentials.js correctly resolves to dist/credentials.js at repo root, matching Vite's [name].js output for the credentials entry.
  • Workflow changes: All references to $GITHUB_WORKSPACE/dist/signed-commit.js and $GITHUB_WORKSPACE/dist/credentials.js are correct; replacing CAGENT_DIR=$(pwd) with $GITHUB_WORKSPACE is actually more robust (survives directory changes in the step).
  • tsconfig.json: Correctly excludes vite.config.ts in place of the deleted vitest.config.ts.
  • Test config consolidation: Vitest project names (unit, integration) match the --project flags in package.json scripts.

@derekmisler derekmisler merged commit 537800e into docker:main Apr 27, 2026
21 checks passed
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.

2 participants