Handle updates to plugin deps in same update (#2243)

In #2242, I mentioned:

> I don't think this handles the case where the update to a dep is in
the same PR as the update to the consuming plugin (I think the syncer
can handle that case?), so we could maybe extend this further to detect
that situation.

This attempts to handle that situation, by refactoring the fetcher a
little bit: we first take a pass to grab all the pending updates, and
then subsequently run the updates (in dependency order), updating the
`latestPluginVersions` as we go so that consumers can be updated with
the latest dependency.

This just needed a tiny bit of tweaking to have an "integration" test
over `run` (making the fetcher an interface), so added that as well.

Author: stefanvanburen

internal/cmd/fetcher/main.go

@@ -32,14 +32,20 @@ var (
 	errNoVersions          = errors.New("no versions found")
 )
 
+// Fetcher is an interface for fetching plugin versions from external sources.
+type Fetcher interface {
+	Fetch(ctx context.Context, config *source.Config) (string, error)
+}
+
 func main() {
 	if len(os.Args) != 2 {
 		_, _ = fmt.Fprintf(os.Stderr, "usage: %s <directory>\n", os.Args)
 		os.Exit(2)
 	}
 	root := os.Args[1]
 	ctx := interrupt.Handle(context.Background())
-	created, err := run(ctx, root)
+	client := fetchclient.New(ctx)
+	created, err := run(ctx, root, client)
 	if err != nil {
 		_, _ = fmt.Fprintf(os.Stderr, "failed to fetch versions: %v\n", err)
 		os.Exit(1)
@@ -288,7 +294,14 @@ func updatePluginDeps(content []byte, latestVersions map[string]string) ([]byte,
 	return updatedContent, nil
 }
 
-func run(ctx context.Context, root string) ([]createdPlugin, error) {
+// pluginToCreate represents a plugin that needs a new version created.
+type pluginToCreate struct {
+	pluginDir       string
+	previousVersion string
+	newVersion      string
+}
+
+func run(ctx context.Context, root string, fetcher Fetcher) ([]createdPlugin, error) {
 	now := time.Now()
 	defer func() {
 		log.Printf("finished running in: %.2fs\n", time.Since(now).Seconds())
@@ -302,12 +315,14 @@ func run(ctx context.Context, root string) ([]createdPlugin, error) {
 		return nil, err
 	}
 
-	// Load all existing plugins to determine latest versions for dependency bumping
+	// Load all existing plugins (already sorted in dependency order by plugin.FindAll)
 	pluginsDir := filepath.Join(root, "plugins")
 	allPlugins, err := plugin.FindAll(pluginsDir)
 	if err != nil {
 		return nil, fmt.Errorf("failed to load existing plugins: %w", err)
 	}
+
+	// Build initial map of latest plugin versions
 	latestPluginVersions := make(map[string]string)
 	for _, p := range allPlugins {
 		current := latestPluginVersions[p.Name]
@@ -320,17 +335,19 @@ func run(ctx context.Context, root string) ([]createdPlugin, error) {
 	if err != nil {
 		return nil, err
 	}
-	client := fetchclient.New(ctx)
+
+	// First pass: fetch all new versions and determine which plugins need updates
 	latestVersions := make(map[string]string, len(configs))
-	created := make([]createdPlugin, 0, len(configs))
+	pendingCreations := make(map[string]*pluginToCreate) // keyed by plugin directory
+
 	for _, config := range configs {
 		if config.Source.Disabled {
 			log.Printf("skipping source: %s", config.Filename)
 			continue
 		}
 		newVersion := latestVersions[config.CacheKey()]
 		if newVersion == "" {
-			newVersion, err = client.Fetch(ctx, config)
+			newVersion, err = fetcher.Fetch(ctx, config)
 			if err != nil {
 				if errors.Is(err, fetchclient.ErrSemverPrerelease) {
 					log.Printf("skipping source: %s: %v", config.Filename, err)
@@ -346,7 +363,6 @@ func run(ctx context.Context, root string) ([]createdPlugin, error) {
 			log.Printf("skipping source: %s: %v", config.Filename, newVersion)
 			continue
 		}
-		// example: plugins/grpc
 		pluginDir := filepath.Dir(config.Filename)
 		ok, err := checkDirExists(filepath.Join(pluginDir, newVersion))
 		if err != nil {
@@ -359,16 +375,40 @@ func run(ctx context.Context, root string) ([]createdPlugin, error) {
 		if err != nil {
 			return nil, fmt.Errorf("failed to get latest known version from dir %s with error: %w", pluginDir, err)
 		}
-		if err := createPluginDir(pluginDir, previousVersion, newVersion, latestBaseImageVersions, latestPluginVersions); err != nil {
-			return nil, err
-		}
-		log.Printf("created %v/%v\n", pluginDir, newVersion)
-		created = append(created, createdPlugin{
-			org:             filepath.Base(filepath.Dir(pluginDir)),
-			name:            filepath.Base(pluginDir),
+
+		pendingCreations[pluginDir] = &pluginToCreate{
 			pluginDir:       pluginDir,
 			previousVersion: previousVersion,
 			newVersion:      newVersion,
+		}
+	}
+
+	// Second pass: create plugins in dependency order (using the order from allPlugins)
+	// Update latestPluginVersions as we go so subsequent plugins reference new versions
+	created := make([]createdPlugin, 0, len(pendingCreations))
+	for _, p := range allPlugins {
+		// Extract the plugin directory from the plugin's path
+		// p.Path is the full path to buf.plugin.yaml, directory is two levels up (dir/version/buf.plugin.yaml)
+		pluginDir := filepath.Dir(filepath.Dir(p.Path))
+		pending, needsCreation := pendingCreations[pluginDir]
+		if !needsCreation {
+			continue
+		}
+
+		if err := createPluginDir(pending.pluginDir, pending.previousVersion, pending.newVersion, latestBaseImageVersions, latestPluginVersions); err != nil {
+			return nil, err
+		}
+		log.Printf("created %v/%v\n", pending.pluginDir, pending.newVersion)
+
+		// Update latestPluginVersions so subsequent plugins in this run can reference this new version
+		latestPluginVersions[p.Name] = pending.newVersion
+
+		created = append(created, createdPlugin{
+			org:             filepath.Base(filepath.Dir(pending.pluginDir)),
+			name:            filepath.Base(pending.pluginDir),
+			pluginDir:       pending.pluginDir,
+			previousVersion: pending.previousVersion,
+			newVersion:      pending.newVersion,
 		})
 	}
 	return created, nil

internal/cmd/fetcher/main_test.go

@@ -1,13 +1,18 @@
 package main
 
 import (
+	"context"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/bufbuild/buf/private/bufpkg/bufremoteplugin/bufremotepluginconfig"
 	"github.com/bufbuild/buf/private/pkg/encoding"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/bufbuild/plugins/internal/source"
 )
 
 func TestUpdatePluginDeps(t *testing.T) {
@@ -138,3 +143,156 @@ plugin_version: v1.0.0
 		})
 	}
 }
+
+// TestRunDependencyOrdering tests the end-to-end behavior of run() with dependency ordering.
+// It verifies that when creating multiple plugin versions in one run, they are processed
+// in dependency order and consumers reference the newly created dependency versions.
+func TestRunDependencyOrdering(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	tmpDir := t.TempDir()
+
+	// Setup: Create complete repository structure
+	setupTestRepository(t, tmpDir)
+
+	// Mock fetcher that returns new versions for our test plugins
+	// Cache keys are formatted as "github-owner-repository"
+	fetcher := &mockFetcher{
+		versions: map[string]string{
+			"github-test-base-plugin":     "v2.0.0",
+			"github-test-consumer-plugin": "v2.0.0",
+		},
+	}
+
+	// Run the fetcher
+	created, err := run(ctx, tmpDir, fetcher)
+	require.NoError(t, err)
+
+	// Verify plugins were created in dependency order
+	require.Len(t, created, 2, "should create 2 new plugin versions")
+	assert.Equal(t, "base-plugin", created[0].name, "base-plugin should be created first (no dependencies)")
+	assert.Equal(t, "v2.0.0", created[0].newVersion)
+	assert.Equal(t, "consumer-plugin", created[1].name, "consumer-plugin should be created second (depends on base-plugin)")
+	assert.Equal(t, "v2.0.0", created[1].newVersion)
+
+	// Verify consumer references the newly created base-plugin v2.0.0
+	consumerYAMLPath := filepath.Join(tmpDir, "plugins", "test", "consumer-plugin", "v2.0.0", "buf.plugin.yaml")
+	content, err := os.ReadFile(consumerYAMLPath)
+	require.NoError(t, err, "should be able to read created consumer plugin config")
+
+	var config bufremotepluginconfig.ExternalConfig
+	err = encoding.UnmarshalJSONOrYAMLStrict(content, &config)
+	require.NoError(t, err, "should be able to parse consumer plugin config")
+
+	require.Len(t, config.Deps, 1, "consumer should have one dependency")
+	assert.Equal(t, "buf.build/test/base-plugin:v2.0.0", config.Deps[0].Plugin,
+		"consumer should reference newly created base-plugin v2.0.0, not the old v1.0.0")
+}
+
+// mockFetcher returns predetermined versions for testing.
+type mockFetcher struct {
+	versions map[string]string // maps cache key (e.g., "github-owner-repo") -> version to return
+}
+
+func (m *mockFetcher) Fetch(_ context.Context, config *source.Config) (string, error) {
+	key := config.CacheKey()
+	if version, ok := m.versions[key]; ok {
+		return version, nil
+	}
+	// Return a default version if not in map
+	return "v1.0.0", nil
+}
+
+// setupTestRepository creates a complete test repository structure with:
+// - plugins/ directory with base-plugin and consumer-plugin
+// - source.yaml files for version detection
+// - .github/docker/ directory with base images.
+func setupTestRepository(t *testing.T, tmpDir string) {
+	t.Helper()
+
+	// Create base Docker images directory (required by run())
+	baseImageDir := filepath.Join(tmpDir, ".github", "docker")
+	require.NoError(t, os.MkdirAll(baseImageDir, 0755))
+
+	// Create required docker/dockerfile base image
+	dockerfileImage := `FROM docker/dockerfile:1.19
+`
+	require.NoError(t, os.WriteFile(filepath.Join(baseImageDir, "Dockerfile.dockerfile"), []byte(dockerfileImage), 0644))
+
+	// Create golang base image
+	golangImage := `FROM golang:1.22.0-bookworm
+`
+	require.NoError(t, os.WriteFile(filepath.Join(baseImageDir, "Dockerfile.golang"), []byte(golangImage), 0644))
+
+	// Setup base-plugin v1.0.0
+	basePluginDir := filepath.Join(tmpDir, "plugins", "test", "base-plugin")
+	require.NoError(t, os.MkdirAll(filepath.Join(basePluginDir, "v1.0.0"), 0755))
+
+	// Create source.yaml for base-plugin
+	baseSourceYAML := `source:
+  github:
+    owner: test
+    repository: base-plugin
+`
+	require.NoError(t, os.WriteFile(filepath.Join(basePluginDir, "source.yaml"), []byte(baseSourceYAML), 0644))
+
+	// Create buf.plugin.yaml for base-plugin v1.0.0
+	basePluginYAML := `version: v1
+name: buf.build/test/base-plugin
+plugin_version: v1.0.0
+output_languages:
+  - go
+`
+	require.NoError(t, os.WriteFile(
+		filepath.Join(basePluginDir, "v1.0.0", "buf.plugin.yaml"),
+		[]byte(basePluginYAML),
+		0644,
+	))
+
+	// Create Dockerfile for base-plugin v1.0.0
+	baseDockerfile := `FROM golang:1.22.0-bookworm
+COPY --from=base /binary /usr/local/bin/protoc-gen-base
+`
+	require.NoError(t, os.WriteFile(
+		filepath.Join(basePluginDir, "v1.0.0", "Dockerfile"),
+		[]byte(baseDockerfile),
+		0644,
+	))
+
+	// Setup consumer-plugin v1.0.0
+	consumerPluginDir := filepath.Join(tmpDir, "plugins", "test", "consumer-plugin")
+	require.NoError(t, os.MkdirAll(filepath.Join(consumerPluginDir, "v1.0.0"), 0755))
+
+	// Create source.yaml for consumer-plugin
+	consumerSourceYAML := `source:
+  github:
+    owner: test
+    repository: consumer-plugin
+`
+	require.NoError(t, os.WriteFile(filepath.Join(consumerPluginDir, "source.yaml"), []byte(consumerSourceYAML), 0644))
+
+	// Create buf.plugin.yaml for consumer-plugin v1.0.0 that depends on base-plugin v1.0.0
+	consumerPluginYAML := `version: v1
+name: buf.build/test/consumer-plugin
+plugin_version: v1.0.0
+deps:
+  - plugin: buf.build/test/base-plugin:v1.0.0
+output_languages:
+  - go
+`
+	require.NoError(t, os.WriteFile(
+		filepath.Join(consumerPluginDir, "v1.0.0", "buf.plugin.yaml"),
+		[]byte(consumerPluginYAML),
+		0644,
+	))
+
+	// Create Dockerfile for consumer-plugin v1.0.0
+	consumerDockerfile := `FROM golang:1.22.0-bookworm
+COPY --from=consumer /binary /usr/local/bin/protoc-gen-consumer
+`
+	require.NoError(t, os.WriteFile(
+		filepath.Join(consumerPluginDir, "v1.0.0", "Dockerfile"),
+		[]byte(consumerDockerfile),
+		0644,
+	))
+}