From 3980dafd2ba44b677566d3a62c09766b1f928ecf Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Mon, 11 Dec 2023 16:39:26 -0500 Subject: [PATCH 01/29] file name cleanup --- private/buf/bufmigrate/bufmigrate.go | 34 +- private/buf/bufmigrate/migrator.go | 327 ++++++++++-------- private/buf/cmd/buf/buf.go | 4 +- .../migratev1.go => migrate/migrate.go} | 11 +- .../beta/{migratev1 => migrate}/usage.gen.go | 2 +- private/bufpkg/bufconfig/buf_lock_file.go | 3 + private/bufpkg/bufconfig/buf_yaml_file.go | 11 +- 7 files changed, 211 insertions(+), 181 deletions(-) rename private/buf/cmd/buf/command/beta/{migratev1/migratev1.go => migrate/migrate.go} (89%) rename private/buf/cmd/buf/command/beta/{migratev1 => migrate}/usage.gen.go (97%) diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index 0292d7e862..2c947fa8d1 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -24,11 +24,13 @@ import ( "buf.build/gen/go/bufbuild/registry/connectrpc/go/buf/registry/module/v1beta1/modulev1beta1connect" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage/storageos" + "go.uber.org/zap" ) // Migrate migrate buf configuration files. func Migrate( ctx context.Context, + logger *zap.Logger, storageProvider storageos.Provider, commitService modulev1beta1connect.CommitServiceClient, options ...MigrateOption, @@ -37,22 +39,10 @@ func Migrate( for _, option := range options { option(migrateOptions) } + // TODO: check buf.gen.yaml as well when it's added if migrateOptions.bufWorkYAMLFilePath == "" && len(migrateOptions.bufYAMLFilePaths) == 0 { - return errors.New("unimplmented") + return errors.New("no ") } - // TODO: decide on behavior for where to write this file - destinationDir := "." - // Alternatively, we could do the following: (but "." is probably better, since we want users to - // have buf.yaml v2 at their repository root and they are likely running this command from there) - // - // if migrateOptions.bufWorkYAMLFilePath != "" { - // destinationDir = filepath.Base(migrateOptions.bufWorkYAMLFilePath) - // } else if len(migrateOptions.bufYAMLFilePaths) == 1 { - // // TODO: maybe use "." (or maybe add --dest flag) - // destinationDir = filepath.Base(migrateOptions.bufYAMLFilePaths[0]) - // } else { - // destinationDir = "." - // } bucket, err := storageProvider.NewReadWriteBucket( ".", storageos.ReadWriteBucketWithSymlinksIfSupported(), @@ -61,13 +51,15 @@ func Migrate( return err } migrator := newMigrator( + logger, bucket, - destinationDir, + ".", ) if migrateOptions.bufWorkYAMLFilePath != "" { - if err := migrator.addWorkspaceDirectory( + // TODO: should we allow multiple workspaces? + if err := migrator.addWorkspaceForBufWorkYAML( ctx, - filepath.Dir(migrateOptions.bufWorkYAMLFilePath), + migrateOptions.bufWorkYAMLFilePath, ); err != nil { return err } @@ -75,9 +67,9 @@ func Migrate( for _, bufYAMLPath := range migrateOptions.bufYAMLFilePaths { // TODO: read upwards to make sure it's not in a workspace // i.e. for ./foo/bar/buf.yaml, check none of "./foo", ".", "../", "../..", and etc. is a workspace. - if err := migrator.addModuleDirectory( + if err := migrator.addModuleDirectoryForBufYAML( ctx, - filepath.Dir(bufYAMLPath), + bufYAMLPath, ); err != nil { return err } @@ -99,7 +91,7 @@ func MigrateAsDryRun(writer io.Writer) MigrateOption { } } -// MigrateBufWorkYAMLFile migrates a v1 buf.work.yaml. +// MigrateBufWorkYAMLFile migrates a buf.work.yaml. func MigrateBufWorkYAMLFile(path string) (MigrateOption, error) { // TODO: Looking at IsLocal's doc, it seems to validate for what we want: relative and does not jump context. if !filepath.IsLocal(path) { @@ -122,6 +114,8 @@ func MigrateBufYAMLFile(paths []string) (MigrateOption, error) { }, nil } +/// *** PRIVATE *** + type migrateOptions struct { dryRun bool dryRunWriter io.Writer diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 383f8e92e8..2fd3886155 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -26,15 +26,19 @@ import ( "strings" "github.com/bufbuild/buf/private/bufpkg/bufcheck" + "github.com/bufbuild/buf/private/bufpkg/bufcheck/bufbreaking" "github.com/bufbuild/buf/private/bufpkg/bufcheck/buflint" "github.com/bufbuild/buf/private/bufpkg/bufconfig" "github.com/bufbuild/buf/private/bufpkg/bufmodule" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/syserror" + "go.uber.org/multierr" + "go.uber.org/zap" ) type migrator struct { + logger *zap.Logger // the directory where the migrated buf.yaml live, this is useful for computing // module directory paths, and possibly other paths. destinationDir string @@ -51,10 +55,12 @@ type migrator struct { } func newMigrator( + logger *zap.Logger, rootBucket storage.ReadWriteBucket, destinationDir string, ) *migrator { return &migrator{ + logger: logger, destinationDir: destinationDir, rootBucket: rootBucket, moduleNameToParentFile: map[string]string{}, @@ -62,18 +68,27 @@ func newMigrator( } } -func (m *migrator) addWorkspaceDirectory( +func (m *migrator) addWorkspaceForBufWorkYAML( ctx context.Context, - workspaceDir string, -) error { - bufWorkYAML, err := bufconfig.GetBufWorkYAMLFileForPrefix(ctx, m.rootBucket, workspaceDir) + bufWorkYAMLPath string, +) (retErr error) { + file, err := os.Open(bufWorkYAMLPath) + if err != nil { + return err + } + defer func() { + retErr = multierr.Append(retErr, file.Close()) + }() + bufWorkYAML, err := bufconfig.ReadBufWorkYAMLFile(file) if err != nil { return err } - // TODO: get path properly - m.seenFiles[filepath.Join(workspaceDir, "buf.work.yaml")] = struct{}{} + m.seenFiles[bufWorkYAMLPath] = struct{}{} + workspaceDir := filepath.Dir(bufWorkYAMLPath) for _, moduleDirRelativeToWorkspace := range bufWorkYAML.DirPaths() { - m.addModuleDirectory(ctx, filepath.Join(workspaceDir, moduleDirRelativeToWorkspace)) + if err := m.addModuleDirectory(ctx, filepath.Join(workspaceDir, moduleDirRelativeToWorkspace)); err != nil { + return err + } } return nil } @@ -84,12 +99,9 @@ func (m *migrator) addModuleDirectory( // moduleDir is the relative path (relative to ".") to the module directory moduleDir string, ) error { - // TODO: get file path properly - bufYAMLPath := filepath.Join(moduleDir, "buf.yaml") - bufYAMLFile, err := bufconfig.GetBufYAMLFileForPrefix( - ctx, - m.rootBucket, - moduleDir, + bufYAMLPath, err := findFirstExistingPath( + filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName), + filepath.Join(moduleDir, bufconfig.LegacyBufYAMLFileName), ) if errors.Is(errors.Unwrap(err), fs.ErrNotExist) { moduleDirInMigratedBufYAML, err := filepath.Rel(m.destinationDir, moduleDir) @@ -106,7 +118,10 @@ func (m *migrator) addModuleDirectory( if err != nil { return err } - if err := m.appendModuleConfig(moduleConfig, bufYAMLPath); err != nil { + if err := m.appendModuleConfig( + moduleConfig, + filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName), + ); err != nil { return err } // Assume there is no co-resident buf.lock @@ -115,140 +130,51 @@ func (m *migrator) addModuleDirectory( if err != nil { return err } - if err := m.addBufYAML( - ctx, - bufYAMLFile, - bufYAMLPath, - ); err != nil { - return err - } - bufLockFile, err := bufconfig.GetBufLockFileForPrefix( - ctx, - m.rootBucket, - moduleDir, - ) - if errors.Is(errors.Unwrap(err), fs.ErrNotExist) { - return nil - } - if err != nil { - return err - } - // TODO: get file paths properly - bufLockFilePath := filepath.Join(moduleDir, "buf.lock") - // We don't need to check whether it's already in the map, but because if it were, - // its co-resident buf.yaml would also have been a duplicate, which would make this - // function return early. - m.seenFiles[bufLockFilePath] = struct{}{} - switch bufLockFile.FileVersion() { - case bufconfig.FileVersionV1Beta1, bufconfig.FileVersionV1: - m.depModuleKeys = append(m.depModuleKeys, bufLockFile.DepModuleKeys()...) - case bufconfig.FileVersionV2: - return fmt.Errorf("%s is already at v2", bufLockFilePath) - default: - return syserror.Newf("unrecognized version: %v", bufLockFile.FileVersion()) - } - return nil + return m.addModuleDirectoryForBufYAML(ctx, bufYAMLPath) } -func (m *migrator) migrateAsDryRun( - writer io.Writer, -) error { - migratedBufYAML, err := m.getBufYAML() - if err != nil { - return err - } - migratedBufLock, err := m.getBufLock() - if err != nil { - return err - } - filesToDelete := m.filesToDelete() - var bufYAMLBuffer bytes.Buffer - if err := bufconfig.WriteBufYAMLFile(&bufYAMLBuffer, migratedBufYAML); err != nil { - return err - } - var bufLockBuffer bytes.Buffer - if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { - return err - } - fmt.Fprintf( - writer, - `in an actual run, these files will be removed: -%s - -these files will be written: -%s: -%s -%s: -%s -`, - strings.Join(filesToDelete, "\n"), - // TODO: find a way to get file name properly - filepath.Join(m.destinationDir, "buf.yaml"), - bufYAMLBuffer.String(), - // TODO: find a way to get file name properly - filepath.Join(m.destinationDir, "buf.lock"), - bufLockBuffer.String(), - ) - return nil -} - -func (m *migrator) migrate( +func (m *migrator) addModuleDirectoryForBufYAML( ctx context.Context, -) error { - migratedBufYAML, err := m.getBufYAML() + bufYAMLPath string, +) (retErr error) { + file, err := os.Open(bufYAMLPath) if err != nil { return err } - migratedBufLock, err := m.getBufLock() + defer func() { + retErr = multierr.Append(retErr, file.Close()) + }() + bufYAML, err := bufconfig.ReadBufYAMLFile(file) if err != nil { return err } - filesToDelete := m.filesToDelete() - for _, fileToDelete := range filesToDelete { - if err := os.Remove(fileToDelete); err != nil { - return err - } - } - if err := bufconfig.PutBufYAMLFileForPrefix( - ctx, - m.rootBucket, - m.destinationDir, - migratedBufYAML, - ); err != nil { - return err - } - if err := bufconfig.PutBufLockFileForPrefix( - ctx, - m.rootBucket, - m.destinationDir, - migratedBufLock, - ); err != nil { - return err - } - return nil -} - -func (m *migrator) addBufYAML( - ctx context.Context, - bufYAML bufconfig.BufYAMLFile, - bufYAMLPath string, -) error { if _, ok := m.seenFiles[bufYAMLPath]; ok { - // TODO: this isn't always the case, perhaps the first time it was read as part of the workspace. - // TODO: we could also return nil here. - return fmt.Errorf("%s is specified multiple times", bufYAMLPath) + return nil } m.seenFiles[bufYAMLPath] = struct{}{} // TODO: transform paths so that they are relative to the new buf.yaml v2 or module root (depending on buf.yaml v2 semantics) + // Paths include RootToExcludes, IgnorePaths, IgnoreIDOrCategoryToPaths switch bufYAML.FileVersion() { case bufconfig.FileVersionV1Beta1: - // TODO: whether something needs to be done about root to exclude mapping (what is it relative to now?) if len(bufYAML.ModuleConfigs()) != 1 { return syserror.Newf("expect exactly 1 module config from buf yaml, got %d", len(bufYAML.ModuleConfigs())) } moduleConfig := bufYAML.ModuleConfigs()[0] - // TODO: iterate through this map in deterministic order - for root, excludes := range moduleConfig.RootToExcludes() { + // If a v1beta buf.yaml has multiple roots, they are split into multiple + // module configs, but they cannot share the same module full name. + moduleFullName := moduleConfig.ModuleFullName() + if len(moduleConfig.RootToExcludes()) > 1 && moduleFullName != nil { + m.logger.Sugar().Warnf( + "%s has name %s and multiple roots. These roots are now separate unnamed modules.", + bufYAMLPath, + moduleFullName.String(), + ) + moduleFullName = nil + } + // Iterate through root-to-excludes in deterministic order. + sortedRoots := slicesext.MapKeysToSortedSlice(moduleConfig.RootToExcludes()) + for _, root := range sortedRoots { + excludes := moduleConfig.RootToExcludes()[root] lintConfig := moduleConfig.LintConfig() // TODO: this list expands to individual rules, we could process // this list and make it shorter by substituting some rules with @@ -276,11 +202,15 @@ func (m *migrator) addBufYAML( lintConfig.AllowCommentIgnores(), ) breakingConfig := moduleConfig.BreakingConfig() + breakingRules, err := bufbreaking.RulesForConfig(breakingConfig) + if err != nil { + return err + } + breakingRuleNames := slicesext.Map(breakingRules, func(rule bufcheck.Rule) string { return rule.ID() }) breakingConfigForRoot := bufconfig.NewBreakingConfig( bufconfig.NewCheckConfig( bufconfig.FileVersionV2, - // TODO: FIELD_SAME_TYPE - breakingConfig.UseIDsAndCategories(), + breakingRuleNames, breakingConfig.ExceptIDsAndCategories(), // TODO: filter these paths by root breakingConfig.IgnorePaths(), @@ -299,24 +229,10 @@ func (m *migrator) addBufYAML( if err != nil { return err } - // If a v1beta buf.yaml has multiple roots, they are split into multiple - // module configs, but they cannot share the same module full name. - moduleFullName := moduleConfig.ModuleFullName() - if len(moduleConfig.RootToExcludes()) > 1 && moduleFullName != nil { - moduleFullName, err = bufmodule.NewModuleFullName( - moduleFullName.Registry(), - moduleFullName.Owner(), - // Note: roots are normalized, "/" is universal - moduleFullName.Name()+"-"+strings.ReplaceAll(root, "/", "-"), - ) - if err != nil { - return err - } - } configForRoot, err := bufconfig.NewModuleConfig( dirPathRelativeToDestination, moduleFullName, - // TODO: excludes might need to be transformed WRT what it's relative to + // TODO: make them relative to what they should be relative to. map[string][]string{".": excludes}, lintConfigForRoot, breakingConfigForRoot, @@ -329,7 +245,6 @@ func (m *migrator) addBufYAML( } } m.moduleDependencies = append(m.moduleDependencies, bufYAML.ConfiguredDepModuleRefs()...) - return nil case bufconfig.FileVersionV1: // TODO: smiliar to the above, make paths (root to excludes, lint ignore, ...) relative to the correct root (either buf.yaml v2 or module root) if len(bufYAML.ModuleConfigs()) != 1 { @@ -385,16 +300,116 @@ func (m *migrator) addBufYAML( return err } m.moduleDependencies = append(m.moduleDependencies, bufYAML.ConfiguredDepModuleRefs()...) - return nil case bufconfig.FileVersionV2: return fmt.Errorf("%s is already at v2", bufYAMLPath) default: return syserror.Newf("unexpected version: %v", bufYAML.FileVersion()) } + moduleDir := filepath.Dir(bufYAMLPath) + bufLockFile, err := bufconfig.GetBufLockFileForPrefix( + ctx, + m.rootBucket, + moduleDir, + ) + if errors.Is(errors.Unwrap(err), fs.ErrNotExist) { + return nil + } + if err != nil { + return err + } + bufLockFilePath := filepath.Join(moduleDir, bufconfig.DefaultBufLockFileName) + // We don't need to check whether it's already in the map, but because if it were, + // its co-resident buf.yaml would also have been a duplicate, which would make this + // function return early. + m.seenFiles[bufLockFilePath] = struct{}{} + switch bufLockFile.FileVersion() { + case bufconfig.FileVersionV1Beta1, bufconfig.FileVersionV1: + m.depModuleKeys = append(m.depModuleKeys, bufLockFile.DepModuleKeys()...) + case bufconfig.FileVersionV2: + return fmt.Errorf("%s is already at v2", bufLockFilePath) + default: + return syserror.Newf("unrecognized version: %v", bufLockFile.FileVersion()) + } + return nil +} + +func (m *migrator) migrateAsDryRun( + writer io.Writer, +) error { + migratedBufYAML, err := m.getBufYAML() + if err != nil { + return err + } + migratedBufLock, err := m.getBufLock() + if err != nil { + return err + } + filesToDelete := m.filesToDelete() + var bufYAMLBuffer bytes.Buffer + if err := bufconfig.WriteBufYAMLFile(&bufYAMLBuffer, migratedBufYAML); err != nil { + return err + } + var bufLockBuffer bytes.Buffer + if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { + return err + } + fmt.Fprintf( + writer, + `in an actual run, these files will be removed: +%s + +these files will be written: +%s: +%s +%s: +%s +`, + strings.Join(filesToDelete, "\n"), + filepath.Join(m.destinationDir, bufconfig.DefaultBufYAMLFileName), + bufYAMLBuffer.String(), + filepath.Join(m.destinationDir, bufconfig.DefaultBufLockFileName), + bufLockBuffer.String(), + ) + return nil +} + +func (m *migrator) migrate( + ctx context.Context, +) error { + migratedBufYAML, err := m.getBufYAML() + if err != nil { + return err + } + migratedBufLock, err := m.getBufLock() + if err != nil { + return err + } + filesToDelete := m.filesToDelete() + for _, fileToDelete := range filesToDelete { + if err := os.Remove(fileToDelete); err != nil { + return err + } + } + if err := bufconfig.PutBufYAMLFileForPrefix( + ctx, + m.rootBucket, + m.destinationDir, + migratedBufYAML, + ); err != nil { + return err + } + if err := bufconfig.PutBufLockFileForPrefix( + ctx, + m.rootBucket, + m.destinationDir, + migratedBufLock, + ); err != nil { + return err + } + return nil } func (m *migrator) getBufYAML() (bufconfig.BufYAMLFile, error) { - // TODO: where do we update seenModuleNames? filteredModuleDependencies := slicesext.Filter( m.moduleDependencies, func(moduleRef bufmodule.ModuleRef) bool { @@ -446,3 +461,17 @@ func (m *migrator) appendModuleConfig(moduleConfig bufconfig.ModuleConfig, paren func (m *migrator) filesToDelete() []string { return slicesext.MapKeysToSortedSlice(m.seenFiles) } + +func findFirstExistingPath(paths ...string) (string, error) { + for _, path := range paths { + _, err := os.Stat(path) + if errors.Is(err, fs.ErrNotExist) { + continue + } + if err != nil { + return "", err + } + return path, nil + } + return "", fs.ErrNotExist +} diff --git a/private/buf/cmd/buf/buf.go b/private/buf/cmd/buf/buf.go index 6eadd862c0..1b43ed1b04 100644 --- a/private/buf/cmd/buf/buf.go +++ b/private/buf/cmd/buf/buf.go @@ -33,7 +33,7 @@ import ( "github.com/bufbuild/buf/private/buf/cmd/buf/command/alpha/registry/token/tokenget" "github.com/bufbuild/buf/private/buf/cmd/buf/command/alpha/registry/token/tokenlist" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/graph" - "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/migratev1" + "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/migrate" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/price" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/registry/commit/commitget" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/registry/commit/commitlist" @@ -141,7 +141,7 @@ func NewRootCommand(name string) *appcmd.Command { Short: "Beta commands. Unstable and likely to change", SubCommands: []*appcmd.Command{ graph.NewCommand("graph", builder), - migratev1.NewCommand("migrate-v1", builder), + migrate.NewCommand("migrate-v1", builder), price.NewCommand("price", builder), stats.NewCommand("stats", builder), studioagent.NewCommand("studio-agent", builder), diff --git a/private/buf/cmd/buf/command/beta/migratev1/migratev1.go b/private/buf/cmd/buf/command/beta/migrate/migrate.go similarity index 89% rename from private/buf/cmd/buf/command/beta/migratev1/migratev1.go rename to private/buf/cmd/buf/command/beta/migrate/migrate.go index 46de21eaa9..62fa79369c 100644 --- a/private/buf/cmd/buf/command/beta/migratev1/migratev1.go +++ b/private/buf/cmd/buf/command/beta/migrate/migrate.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package migratev1 +package migrate import ( "context" @@ -25,7 +25,6 @@ import ( ) const ( - // TODO: buf-work-yaml or bufwork-yaml? bufWorkYAMLPathFlagName = "buf-work-yaml" bufYAMLPathsFlagName = "buf-yaml" dryRunFlagName = "dry-run" @@ -38,13 +37,10 @@ func NewCommand( ) *appcmd.Command { flags := newFlags() return &appcmd.Command{ - // TODO: update if we are taking a directory as input Use: name, Short: `Migrate configuration to the latest version`, - Long: `Migrate any v1 and v1beta1 configuration files in the directory to the latest version. -Defaults to the current directory if not specified.`, - // TODO: update if we are taking a directory as input - Args: appcmd.MaximumNArgs(0), + Long: `Migrate specified configuration files to the latest version.`, + Args: appcmd.MaximumNArgs(0), Run: builder.NewRunFunc( func(ctx context.Context, container appext.Container) error { return run(ctx, container, flags) @@ -110,6 +106,7 @@ func run( } return bufmigrate.Migrate( ctx, + container.Logger(), // TODO: Do we want to add a flag --disable-symlinks? storageos.NewProvider(storageos.ProviderWithSymlinks()), // TODO: pass an actual client when implemented on server diff --git a/private/buf/cmd/buf/command/beta/migratev1/usage.gen.go b/private/buf/cmd/buf/command/beta/migrate/usage.gen.go similarity index 97% rename from private/buf/cmd/buf/command/beta/migratev1/usage.gen.go rename to private/buf/cmd/buf/command/beta/migrate/usage.gen.go index b712e224a2..8e47f3bd91 100644 --- a/private/buf/cmd/buf/command/beta/migratev1/usage.gen.go +++ b/private/buf/cmd/buf/command/beta/migrate/usage.gen.go @@ -14,6 +14,6 @@ // Generated. DO NOT EDIT. -package migratev1 +package migrate import _ "github.com/bufbuild/buf/private/usage" diff --git a/private/bufpkg/bufconfig/buf_lock_file.go b/private/bufpkg/bufconfig/buf_lock_file.go index 3c28ccd1e0..4ae924275c 100644 --- a/private/bufpkg/bufconfig/buf_lock_file.go +++ b/private/bufpkg/bufconfig/buf_lock_file.go @@ -31,6 +31,9 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) +// DefaultBufLockFileName is the buf.yaml default file name. +const DefaultBufLockFileName = "buf.yaml" + var ( // bufLockFileHeader is the header prepended to any lock files. bufLockFileHeader = []byte("# Generated by buf. DO NOT EDIT.\n") diff --git a/private/bufpkg/bufconfig/buf_yaml_file.go b/private/bufpkg/bufconfig/buf_yaml_file.go index ee138f3af4..4a7ed6bb5d 100644 --- a/private/bufpkg/bufconfig/buf_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_yaml_file.go @@ -34,12 +34,19 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) +const ( + // DefaultBufYAMLFileName is the buf.yaml default file name. + DefaultBufYAMLFileName = "buf.yaml" + // LegacyBufYAMLFileName is the old file name for buf.yaml. + LegacyBufYAMLFileName = "buf.mod" +) + var ( - bufYAML = newFileName("buf.yaml", FileVersionV1Beta1, FileVersionV1, FileVersionV2) + bufYAML = newFileName(DefaultBufYAMLFileName, FileVersionV1Beta1, FileVersionV1, FileVersionV2) // Originally we thought we were going to move to buf.mod, and had this around for // a while, but then reverted back to buf.yaml. We still need to support buf.mod as // we released with it, however. - bufMod = newFileName("buf.mod", FileVersionV1Beta1, FileVersionV1) + bufMod = newFileName(LegacyBufYAMLFileName, FileVersionV1Beta1, FileVersionV1) bufYAMLFileNames = []*fileName{bufYAML, bufMod} ) From 736f424bf0bf434ef136b9c3b7258d7567b75d8b Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Mon, 11 Dec 2023 19:08:07 -0500 Subject: [PATCH 02/29] resolve, probably dosen't work because it hasn't been tried with the actual API --- private/buf/bufmigrate/bufmigrate.go | 7 +- private/buf/bufmigrate/migrator.go | 252 +++++++++++++++--- .../cmd/buf/command/beta/migrate/migrate.go | 9 +- 3 files changed, 230 insertions(+), 38 deletions(-) diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index 2c947fa8d1..f1e0ba8db2 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -21,7 +21,7 @@ import ( "io" "path/filepath" - "buf.build/gen/go/bufbuild/registry/connectrpc/go/buf/registry/module/v1beta1/modulev1beta1connect" + "github.com/bufbuild/buf/private/bufpkg/bufapi" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage/storageos" "go.uber.org/zap" @@ -32,7 +32,7 @@ func Migrate( ctx context.Context, logger *zap.Logger, storageProvider storageos.Provider, - commitService modulev1beta1connect.CommitServiceClient, + clientProvider bufapi.ClientProvider, options ...MigrateOption, ) (retErr error) { migrateOptions := newMigrateOptions() @@ -52,6 +52,7 @@ func Migrate( } migrator := newMigrator( logger, + clientProvider, bucket, ".", ) @@ -75,7 +76,7 @@ func Migrate( } } if migrateOptions.dryRun { - return migrator.migrateAsDryRun(migrateOptions.dryRunWriter) + return migrator.migrateAsDryRun(ctx, migrateOptions.dryRunWriter) } return migrator.migrate(ctx) } diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 2fd3886155..7b964d9fb6 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -23,8 +23,13 @@ import ( "io/fs" "os" "path/filepath" + "sort" "strings" + modulev1beta1 "buf.build/gen/go/bufbuild/registry/protocolbuffers/go/buf/registry/module/v1beta1" + "connectrpc.com/connect" + "github.com/bufbuild/buf/private/bufpkg/bufapi" + "github.com/bufbuild/buf/private/bufpkg/bufcas" "github.com/bufbuild/buf/private/bufpkg/bufcheck" "github.com/bufbuild/buf/private/bufpkg/bufcheck/bufbreaking" "github.com/bufbuild/buf/private/bufpkg/bufcheck/buflint" @@ -38,7 +43,8 @@ import ( ) type migrator struct { - logger *zap.Logger + logger *zap.Logger + clientProvider bufapi.ClientProvider // the directory where the migrated buf.yaml live, this is useful for computing // module directory paths, and possibly other paths. destinationDir string @@ -56,11 +62,13 @@ type migrator struct { func newMigrator( logger *zap.Logger, + clientProvider bufapi.ClientProvider, rootBucket storage.ReadWriteBucket, destinationDir string, ) *migrator { return &migrator{ logger: logger, + clientProvider: clientProvider, destinationDir: destinationDir, rootBucket: rootBucket, moduleNameToParentFile: map[string]string{}, @@ -334,13 +342,10 @@ func (m *migrator) addModuleDirectoryForBufYAML( } func (m *migrator) migrateAsDryRun( + ctx context.Context, writer io.Writer, ) error { - migratedBufYAML, err := m.getBufYAML() - if err != nil { - return err - } - migratedBufLock, err := m.getBufLock() + migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) if err != nil { return err } @@ -376,11 +381,7 @@ these files will be written: func (m *migrator) migrate( ctx context.Context, ) error { - migratedBufYAML, err := m.getBufYAML() - if err != nil { - return err - } - migratedBufLock, err := m.getBufLock() + migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) if err != nil { return err } @@ -409,7 +410,10 @@ func (m *migrator) migrate( return nil } -func (m *migrator) getBufYAML() (bufconfig.BufYAMLFile, error) { +func (m *migrator) buildBufYAMLAndBufLock( + ctx context.Context, +) (bufconfig.BufYAMLFile, bufconfig.BufLockFile, error) { + // Remove declared dependencies that are also modules in this workspace. filteredModuleDependencies := slicesext.Filter( m.moduleDependencies, func(moduleRef bufmodule.ModuleRef) bool { @@ -417,33 +421,44 @@ func (m *migrator) getBufYAML() (bufconfig.BufYAMLFile, error) { return !ok }, ) - // TODO: deduplicate/resolve dependency list - return bufconfig.NewBufYAMLFile( + // TODO: enable this when ready + moduleRefs := filteredModuleDependencies + var moduleKeys []bufmodule.ModuleKey + if false { + moduleToRefToCommit, err := getModuleToRefToCommit(ctx, filteredModuleDependencies, m.clientProvider) + if err != nil { + return nil, nil, err + } + commitIDToCommit, err := getCommitIDToCommit(ctx, m.clientProvider, m.depModuleKeys) + if err != nil { + return nil, nil, err + } + moduleRefs, moduleKeys, err = resolvedDeclaredAndLockedDependencies( + moduleToRefToCommit, + commitIDToCommit, + filteredModuleDependencies, + m.depModuleKeys, + ) + if err != nil { + return nil, nil, err + } + } + bufYAML, err := bufconfig.NewBufYAMLFile( bufconfig.FileVersionV2, m.moduleConfigs, - filteredModuleDependencies, + moduleRefs, ) -} - -func (m *migrator) getBufLock() (bufconfig.BufLockFile, error) { - depModuleFullNameToModuleKeys := make(map[string][]bufmodule.ModuleKey) - for _, depModuleKey := range m.depModuleKeys { - depModuleFullName := depModuleKey.ModuleFullName().String() - depModuleFullNameToModuleKeys[depModuleFullName] = append(depModuleFullNameToModuleKeys[depModuleFullName], depModuleKey) - } - // TODO: these are resolved arbitrarily right now, we need to resolve them by commit time - resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleFullNameToModuleKeys)) - for _, depModuleKeys := range depModuleFullNameToModuleKeys { - // TODO: actually resolve dependencies by time - // The alternative is to build the workspace with tentative dependencies and - // find the latest one that does not break. However, what if there are 3 dependencies - // in question, each has 4 potential versions. We don't want to build 4*4*4 times in the worst case. - resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys[0]) + if err != nil { + return nil, nil, err } - return bufconfig.NewBufLockFile( + bufLock, err := bufconfig.NewBufLockFile( bufconfig.FileVersionV2, - resolvedDepModuleKeys, + moduleKeys, ) + if err != nil { + return nil, nil, err + } + return bufYAML, bufLock, nil } func (m *migrator) appendModuleConfig(moduleConfig bufconfig.ModuleConfig, parentFile string) error { @@ -475,3 +490,174 @@ func findFirstExistingPath(paths ...string) (string, error) { } return "", fs.ErrNotExist } + +func resolvedDeclaredAndLockedDependencies( + moduleToRefToCommit map[string]map[string]*modulev1beta1.Commit, + commitIDToCommit map[string]*modulev1beta1.Commit, + declaredDependencies []bufmodule.ModuleRef, + lockedDependencies []bufmodule.ModuleKey, +) ([]bufmodule.ModuleRef, []bufmodule.ModuleKey, error) { + depModuleFullNameToRefs := make(map[string][]bufmodule.ModuleRef) + for _, depModuleRef := range declaredDependencies { + moduleFullName := depModuleRef.ModuleFullName().String() + depModuleFullNameToRefs[moduleFullName] = append(depModuleFullNameToRefs[moduleFullName], depModuleRef) + } + depModuleFullNameToResolvedRef := make(map[string]bufmodule.ModuleRef) + for moduleFullName, refs := range depModuleFullNameToRefs { + nonEmptyRef := slicesext.Filter( + refs, + func(ref bufmodule.ModuleRef) bool { + return ref.Ref() != "" + }, + ) + switch len(nonEmptyRef) { + case 0: + // All refs are empty, we take the first one (they are all the same). refs is guaranteed not empty, + // by the construction of depModuleFullNameToRefs. + depModuleFullNameToResolvedRef[moduleFullName] = refs[0] + default: + // There are multiple pinned versions of the same dependency, we use the latest one. + sort.Slice(nonEmptyRef, func(i, j int) bool { + refToCommit := moduleToRefToCommit[moduleFullName] + iTime := refToCommit[refs[i].Ref()].GetCreateTime().AsTime() + jTime := refToCommit[refs[j].Ref()].GetCreateTime().AsTime() + return iTime.After(jTime) + }) + depModuleFullNameToResolvedRef[moduleFullName] = nonEmptyRef[0] + } + } + // We only want locked dependencies that correspond to declared dependencies. + lockedDependencies = slicesext.Filter( + lockedDependencies, + func(lockedDependency bufmodule.ModuleKey) bool { + _, ok := depModuleFullNameToRefs[lockedDependency.ModuleFullName().String()] + return ok + }, + ) + depModuleFullNameToModuleKeys := make(map[string][]bufmodule.ModuleKey) + for _, depModuleKey := range lockedDependencies { + depModuleFullName := depModuleKey.ModuleFullName().String() + depModuleFullNameToModuleKeys[depModuleFullName] = append(depModuleFullNameToModuleKeys[depModuleFullName], depModuleKey) + } + resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleFullNameToModuleKeys)) + for moduleFullName, depModuleKeys := range depModuleFullNameToModuleKeys { + resolvedRef := depModuleFullNameToResolvedRef[moduleFullName] + if resolvedRef.Ref() != "" { + // TODO: do we want to do all this? It feels like we are doing a partial `buf mod update`. + // More specifically, it's possible that the declared dependency pin does not have a corresponding + // lock entry. For example, the buf.lock might not exist. + // Might as well do this for all lock entries. + resolvedCommit := moduleToRefToCommit[moduleFullName][resolvedRef.Ref()] + key, err := bufmodule.NewModuleKey( + resolvedRef.ModuleFullName(), + resolvedCommit.GetId(), + func() (bufcas.Digest, error) { return bufcas.ProtoToDigest(resolvedCommit.GetDigest()) }, + ) + if err != nil { + return nil, nil, err + } + resolvedDepModuleKeys = append(resolvedDepModuleKeys, key) + continue + } + // Use the lastest + sort.Slice(depModuleKeys, func(i, j int) bool { + iTime := commitIDToCommit[depModuleKeys[i].CommitID()].GetCreateTime().AsTime() + jTime := commitIDToCommit[depModuleKeys[j].CommitID()].GetCreateTime().AsTime() + return iTime.After(jTime) + }) + resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys[0]) + } + resolvedDeclaredDependencies := slicesext.MapValuesToSlice(depModuleFullNameToResolvedRef) + sort.Slice(resolvedDeclaredDependencies, func(i, j int) bool { + return resolvedDeclaredDependencies[i].ModuleFullName().String() < resolvedDeclaredDependencies[j].ModuleFullName().String() + }) + sort.Slice(resolvedDepModuleKeys, func(i, j int) bool { + return resolvedDepModuleKeys[i].ModuleFullName().String() < resolvedDepModuleKeys[j].ModuleFullName().String() + }) + return resolvedDeclaredDependencies, resolvedDepModuleKeys, nil +} + +func getModuleToRefToCommit( + ctx context.Context, + moduleRefs []bufmodule.ModuleRef, + clientProvider bufapi.ClientProvider, +) (map[string]map[string]*modulev1beta1.Commit, error) { + moduleToRefToCommit := make(map[string]map[string]*modulev1beta1.Commit) + for _, moduleRef := range moduleRefs { + if moduleRef.Ref() == "" { + continue + } + moduleFullName := moduleRef.ModuleFullName() + response, err := clientProvider.CommitServiceClient(moduleFullName.Registry()).ResolveCommits( + ctx, + connect.NewRequest( + &modulev1beta1.ResolveCommitsRequest{ + ResourceRefs: []*modulev1beta1.ResourceRef{ + { + Value: &modulev1beta1.ResourceRef_Name_{ + Name: &modulev1beta1.ResourceRef_Name{ + Owner: moduleFullName.Owner(), + Module: moduleFullName.Name(), + Child: &modulev1beta1.ResourceRef_Name_Ref{ + Ref: moduleRef.Ref(), + }, + }, + }, + }, + }, + }, + ), + ) + if err != nil { + if connect.CodeOf(err) == connect.CodeNotFound { + return nil, &fs.PathError{Op: "read", Path: moduleRef.String(), Err: fs.ErrNotExist} + } + return nil, err + } + if len(response.Msg.Commits) != 1 { + return nil, fmt.Errorf("expected 1 Commit, got %d", len(response.Msg.Commits)) + } + if moduleToRefToCommit[moduleFullName.String()] == nil { + moduleToRefToCommit[moduleFullName.String()] = make(map[string]*modulev1beta1.Commit) + } + moduleToRefToCommit[moduleFullName.String()][moduleRef.Ref()] = response.Msg.Commits[0] + } + return moduleToRefToCommit, nil +} + +func getCommitIDToCommit( + ctx context.Context, + clientProvider bufapi.ClientProvider, + moduleKeys []bufmodule.ModuleKey, +) (map[string]*modulev1beta1.Commit, error) { + commitIDToCommit := make(map[string]*modulev1beta1.Commit) + for _, moduleKey := range moduleKeys { + moduleFullName := moduleKey.ModuleFullName() + response, err := clientProvider.CommitServiceClient(moduleFullName.Registry()).ResolveCommits( + ctx, + connect.NewRequest( + &modulev1beta1.ResolveCommitsRequest{ + ResourceRefs: []*modulev1beta1.ResourceRef{ + { + Value: &modulev1beta1.ResourceRef_Id{ + // TODO: is this in the correct dashless/dashful form? + Id: moduleKey.CommitID(), + }, + }, + }, + }, + ), + ) + if err != nil { + if connect.CodeOf(err) == connect.CodeNotFound { + return nil, &fs.PathError{Op: "read", Path: moduleKey.CommitID(), Err: fs.ErrNotExist} + } + return nil, err + } + if len(response.Msg.Commits) != 1 { + return nil, fmt.Errorf("expected 1 Commit, got %d", len(response.Msg.Commits)) + } + commitIDToCommit[moduleKey.CommitID()] = response.Msg.Commits[0] + } + return commitIDToCommit, nil +} diff --git a/private/buf/cmd/buf/command/beta/migrate/migrate.go b/private/buf/cmd/buf/command/beta/migrate/migrate.go index 62fa79369c..8ed5fd7459 100644 --- a/private/buf/cmd/buf/command/beta/migrate/migrate.go +++ b/private/buf/cmd/buf/command/beta/migrate/migrate.go @@ -17,7 +17,9 @@ package migrate import ( "context" + "github.com/bufbuild/buf/private/buf/bufcli" "github.com/bufbuild/buf/private/buf/bufmigrate" + "github.com/bufbuild/buf/private/bufpkg/bufapi" "github.com/bufbuild/buf/private/pkg/app/appcmd" "github.com/bufbuild/buf/private/pkg/app/appext" "github.com/bufbuild/buf/private/pkg/storage/storageos" @@ -104,13 +106,16 @@ func run( if flags.DryRun { migrateOptions = append(migrateOptions, bufmigrate.MigrateAsDryRun(container.Stdout())) } + clientConfig, err := bufcli.NewConnectClientConfig(container) + if err != nil { + return err + } return bufmigrate.Migrate( ctx, container.Logger(), // TODO: Do we want to add a flag --disable-symlinks? storageos.NewProvider(storageos.ProviderWithSymlinks()), - // TODO: pass an actual client when implemented on server - nil, + bufapi.NewClientProvider(clientConfig), migrateOptions..., ) } From eb62f9af9afa9cbb16c076dc6d396d78a10d8540 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 12:38:45 -0500 Subject: [PATCH 03/29] doesn't compile --- private/buf/bufmigrate/bufmigrate.go | 50 +++++++++++-------- private/buf/bufmigrate/migrator.go | 41 ++++++++------- .../cmd/buf/command/beta/migrate/migrate.go | 48 +++++++++++------- private/bufpkg/bufconfig/buf_lock_file.go | 4 +- .../bufpkg/bufconfig/buf_work_yaml_file.go | 7 +++ 5 files changed, 93 insertions(+), 57 deletions(-) diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index f1e0ba8db2..d73ce05b3f 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -40,7 +40,7 @@ func Migrate( option(migrateOptions) } // TODO: check buf.gen.yaml as well when it's added - if migrateOptions.bufWorkYAMLFilePath == "" && len(migrateOptions.bufYAMLFilePaths) == 0 { + if migrateOptions.workspaceDirectory == "" && len(migrateOptions.moduleDirectories) == 0 { return errors.New("no ") } bucket, err := storageProvider.NewReadWriteBucket( @@ -56,19 +56,19 @@ func Migrate( bucket, ".", ) - if migrateOptions.bufWorkYAMLFilePath != "" { + if migrateOptions.workspaceDirectory != "" { // TODO: should we allow multiple workspaces? - if err := migrator.addWorkspaceForBufWorkYAML( + if err := migrator.addWorkspace( ctx, - migrateOptions.bufWorkYAMLFilePath, + migrateOptions.workspaceDirectory, ); err != nil { return err } } - for _, bufYAMLPath := range migrateOptions.bufYAMLFilePaths { + for _, bufYAMLPath := range migrateOptions.moduleDirectories { // TODO: read upwards to make sure it's not in a workspace // i.e. for ./foo/bar/buf.yaml, check none of "./foo", ".", "../", "../..", and etc. is a workspace. - if err := migrator.addModuleDirectoryForBufYAML( + if err := migrator.addModuleDirectory( ctx, bufYAMLPath, ); err != nil { @@ -84,7 +84,8 @@ func Migrate( // MigrateOption is a migrate option. type MigrateOption func(*migrateOptions) -// MigrateAsDryRun write to the writer the summary of the changes to be made, without writing to the disk. +// MigrateAsDryRun write to the writer the summary of the changes to be made, +// without writing to the disk. func MigrateAsDryRun(writer io.Writer) MigrateOption { return func(migrateOptions *migrateOptions) { migrateOptions.dryRun = true @@ -92,36 +93,45 @@ func MigrateAsDryRun(writer io.Writer) MigrateOption { } } -// MigrateBufWorkYAMLFile migrates a buf.work.yaml. -func MigrateBufWorkYAMLFile(path string) (MigrateOption, error) { +// MigrateWorkspaceDirectory migrates buf.work.yaml, and all buf.yamls in directories +// pointed to by this workspace, as well as their co-resident buf.locks. +func MigrateWorkspaceDirectory(directory string) (MigrateOption, error) { // TODO: Looking at IsLocal's doc, it seems to validate for what we want: relative and does not jump context. - if !filepath.IsLocal(path) { - return nil, fmt.Errorf("%s is not a relative path", path) + if !filepath.IsLocal(directory) { + return nil, fmt.Errorf("%s is not a relative path", directory) } return func(migrateOptions *migrateOptions) { - migrateOptions.bufWorkYAMLFilePath = filepath.Clean(path) + migrateOptions.workspaceDirectory = filepath.Clean(directory) }, nil } -// MigrateBufYAMLFile migrates buf.yaml files. -func MigrateBufYAMLFile(paths []string) (MigrateOption, error) { - for _, path := range paths { +// MigrateModuleDirectories migrates buf.yamls buf.locks in directories. +func MigrateModuleDirectories(directories []string) (MigrateOption, error) { + for _, path := range directories { if !filepath.IsLocal(path) { return nil, fmt.Errorf("%s is not a relative path", path) } } return func(migrateOptions *migrateOptions) { - migrateOptions.bufYAMLFilePaths = slicesext.Map(paths, filepath.Clean) + migrateOptions.moduleDirectories = slicesext.Map(directories, filepath.Clean) }, nil } +// MigrateGenerationTemplates migrates buf.gen.yamls. +func MigrateGenerationTemplates(paths []string) MigrateOption { + return func(migrateOptions *migrateOptions) { + migrateOptions.bufGenYAMLPaths = slicesext.Map(paths, filepath.Clean) + } +} + /// *** PRIVATE *** type migrateOptions struct { - dryRun bool - dryRunWriter io.Writer - bufWorkYAMLFilePath string - bufYAMLFilePaths []string + dryRun bool + dryRunWriter io.Writer + workspaceDirectory string + moduleDirectories []string + bufGenYAMLPaths []string } func newMigrateOptions() *migrateOptions { diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 7b964d9fb6..8c05f57922 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -76,25 +76,37 @@ func newMigrator( } } -func (m *migrator) addWorkspaceForBufWorkYAML( +func (m *migrator) addWorkspace( ctx context.Context, - bufWorkYAMLPath string, + workspaceDirectory string, ) (retErr error) { + bufWorkYAMLPath, err := findFirstExistingPath( + filepath.Join(workspaceDirectory, bufconfig.DefaultBufWorkYAMLFileName), + filepath.Join(workspaceDirectory, bufconfig.LegacyBufWorkYAMLFileName), + ) + if errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf( + "workspace %q does not have a %s or %s", + workspaceDirectory, + bufconfig.DefaultBufWorkYAMLFileName, + bufconfig.LegacyBufWorkYAMLFileName, + ) + } + if err != nil { + return err + } file, err := os.Open(bufWorkYAMLPath) + // Not checking if err is fs.ErrNotExist because we just did that. if err != nil { return err } - defer func() { - retErr = multierr.Append(retErr, file.Close()) - }() bufWorkYAML, err := bufconfig.ReadBufWorkYAMLFile(file) if err != nil { return err } m.seenFiles[bufWorkYAMLPath] = struct{}{} - workspaceDir := filepath.Dir(bufWorkYAMLPath) for _, moduleDirRelativeToWorkspace := range bufWorkYAML.DirPaths() { - if err := m.addModuleDirectory(ctx, filepath.Join(workspaceDir, moduleDirRelativeToWorkspace)); err != nil { + if err := m.addModuleDirectory(ctx, filepath.Join(workspaceDirectory, moduleDirRelativeToWorkspace)); err != nil { return err } } @@ -106,7 +118,7 @@ func (m *migrator) addModuleDirectory( ctx context.Context, // moduleDir is the relative path (relative to ".") to the module directory moduleDir string, -) error { +) (retErr error) { bufYAMLPath, err := findFirstExistingPath( filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName), filepath.Join(moduleDir, bufconfig.LegacyBufYAMLFileName), @@ -138,14 +150,8 @@ func (m *migrator) addModuleDirectory( if err != nil { return err } - return m.addModuleDirectoryForBufYAML(ctx, bufYAMLPath) -} - -func (m *migrator) addModuleDirectoryForBufYAML( - ctx context.Context, - bufYAMLPath string, -) (retErr error) { file, err := os.Open(bufYAMLPath) + // Not checking if err is fs.ErrNotExist because we just did that. if err != nil { return err } @@ -313,7 +319,6 @@ func (m *migrator) addModuleDirectoryForBufYAML( default: return syserror.Newf("unexpected version: %v", bufYAML.FileVersion()) } - moduleDir := filepath.Dir(bufYAMLPath) bufLockFile, err := bufconfig.GetBufLockFileForPrefix( ctx, m.rootBucket, @@ -327,8 +332,8 @@ func (m *migrator) addModuleDirectoryForBufYAML( } bufLockFilePath := filepath.Join(moduleDir, bufconfig.DefaultBufLockFileName) // We don't need to check whether it's already in the map, but because if it were, - // its co-resident buf.yaml would also have been a duplicate, which would make this - // function return early. + // its co-resident buf.yaml would also have been a duplicate and made this + // function return at an earlier point. m.seenFiles[bufLockFilePath] = struct{}{} switch bufLockFile.FileVersion() { case bufconfig.FileVersionV1Beta1, bufconfig.FileVersionV1: diff --git a/private/buf/cmd/buf/command/beta/migrate/migrate.go b/private/buf/cmd/buf/command/beta/migrate/migrate.go index 8ed5fd7459..e0a6aa5056 100644 --- a/private/buf/cmd/buf/command/beta/migrate/migrate.go +++ b/private/buf/cmd/buf/command/beta/migrate/migrate.go @@ -27,9 +27,10 @@ import ( ) const ( - bufWorkYAMLPathFlagName = "buf-work-yaml" - bufYAMLPathsFlagName = "buf-yaml" - dryRunFlagName = "dry-run" + workspaceDirectoryFlagName = "workspace-dir" + moduleDirectoriesFlagName = "module-dir" + bufGenYAMLPathFlagName = "generate-template" + dryRunFlagName = "dry-run" ) // NewCommand returns a new Command. @@ -53,9 +54,10 @@ func NewCommand( } type flags struct { - BufWorkYAMLPath string - BufYAMLPaths []string - DryRun bool + WorkspaceDirectory string + ModuleDirectories []string + BufGenYAMLPath []string + DryRun bool } func newFlags() *flags { @@ -64,22 +66,28 @@ func newFlags() *flags { func (f *flags) Bind(flagSet *pflag.FlagSet) { flagSet.StringVar( - &f.BufWorkYAMLPath, - bufWorkYAMLPathFlagName, + &f.WorkspaceDirectory, + workspaceDirectoryFlagName, "", - "The buf.work.yaml to migrate", + "The workspace directory to migrate. Its buf.work.yaml, buf.yamls and buf.locks will be migrated.", ) flagSet.StringSliceVar( - &f.BufYAMLPaths, - bufYAMLPathsFlagName, + &f.ModuleDirectories, + moduleDirectoriesFlagName, nil, - "The buf.yamls to migrate. Must not be included by the workspace specified", + "The buf.yamls to migrate. Its buf.yaml and buf.lock will be migrated", ) flagSet.BoolVar( &f.DryRun, dryRunFlagName, false, - "Print the changes to be made, without writing to the disk", + "Print the changes to be made without writing to the disk", + ) + flagSet.StringSliceVar( + &f.BufGenYAMLPath, + bufGenYAMLPathFlagName, + nil, + "The paths to the generation templates to migrate", ) } @@ -89,20 +97,26 @@ func run( flags *flags, ) error { var migrateOptions []bufmigrate.MigrateOption - if flags.BufWorkYAMLPath != "" { - option, err := bufmigrate.MigrateBufWorkYAMLFile(flags.BufWorkYAMLPath) + if flags.WorkspaceDirectory != "" { + option, err := bufmigrate.MigrateWorkspaceDirectory(flags.WorkspaceDirectory) if err != nil { return err } migrateOptions = append(migrateOptions, option) } - if len(flags.BufYAMLPaths) > 0 { - option, err := bufmigrate.MigrateBufYAMLFile(flags.BufYAMLPaths) + if len(flags.ModuleDirectories) > 0 { + option, err := bufmigrate.MigrateModuleDirectories(flags.ModuleDirectories) if err != nil { return err } migrateOptions = append(migrateOptions, option) } + if len(flags.BufGenYAMLPath) > 0 { + migrateOptions = append( + migrateOptions, + bufmigrate.MigrateGenerationTemplates(flags.BufGenYAMLPath), + ) + } if flags.DryRun { migrateOptions = append(migrateOptions, bufmigrate.MigrateAsDryRun(container.Stdout())) } diff --git a/private/bufpkg/bufconfig/buf_lock_file.go b/private/bufpkg/bufconfig/buf_lock_file.go index 4ae924275c..b30e6d8159 100644 --- a/private/bufpkg/bufconfig/buf_lock_file.go +++ b/private/bufpkg/bufconfig/buf_lock_file.go @@ -31,8 +31,8 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) -// DefaultBufLockFileName is the buf.yaml default file name. -const DefaultBufLockFileName = "buf.yaml" +// DefaultBufLockFileName is the buf.lock default file name. +const DefaultBufLockFileName = "buf.lock" var ( // bufLockFileHeader is the header prepended to any lock files. diff --git a/private/bufpkg/bufconfig/buf_work_yaml_file.go b/private/bufpkg/bufconfig/buf_work_yaml_file.go index 009a677459..934b958eae 100644 --- a/private/bufpkg/bufconfig/buf_work_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_work_yaml_file.go @@ -27,6 +27,13 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) +const ( + // DefaultBufWorkYAMLFileName is the default buf.work.yaml file name. + DefaultBufWorkYAMLFileName = "buf.work.yaml" + // LegacyBufWorkYAMLFileName is the legacy buf.work file name. + LegacyBufWorkYAMLFileName = "buf.work" +) + var ( // We only supported buf.work.yamls in v1. bufWorkYAML = newFileName("buf.work.yaml", FileVersionV1) From c176f360b06fd591bcc10c5d1105d7f860cc5ec3 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 13:56:21 -0500 Subject: [PATCH 04/29] add FileName to buf lock, buf yaml and buf work yaml --- private/bufpkg/bufconfig/buf_gen_yaml_file.go | 2 +- private/bufpkg/bufconfig/buf_lock_file.go | 31 +++++++++++--- .../bufpkg/bufconfig/buf_work_yaml_file.go | 29 ++++++++++---- private/bufpkg/bufconfig/buf_yaml_file.go | 40 +++++++++++++++---- private/bufpkg/bufconfig/file.go | 24 +++++++---- 5 files changed, 96 insertions(+), 30 deletions(-) diff --git a/private/bufpkg/bufconfig/buf_gen_yaml_file.go b/private/bufpkg/bufconfig/buf_gen_yaml_file.go index 97608eb186..07d6c454cc 100644 --- a/private/bufpkg/bufconfig/buf_gen_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_gen_yaml_file.go @@ -140,7 +140,7 @@ func (g *bufGenYAMLFile) InputConfigs() []InputConfig { func (*bufGenYAMLFile) isBufGenYAMLFile() {} func (*bufGenYAMLFile) isFile() {} -func readBufGenYAMLFile(reader io.Reader, allowJSON bool) (BufGenYAMLFile, error) { +func readBufGenYAMLFile(reader io.Reader, _ string, allowJSON bool) (BufGenYAMLFile, error) { data, err := io.ReadAll(reader) if err != nil { return nil, err diff --git a/private/bufpkg/bufconfig/buf_lock_file.go b/private/bufpkg/bufconfig/buf_lock_file.go index 3c28ccd1e0..6deaa98777 100644 --- a/private/bufpkg/bufconfig/buf_lock_file.go +++ b/private/bufpkg/bufconfig/buf_lock_file.go @@ -48,6 +48,11 @@ var ( type BufLockFile interface { File + // FileName returns the name of the file when the BufLockFile was read. + // + // If there was no file name, this returns empty. This can happen when the BufLockFile + // is created via ReadBufLockFile, for example. + FileName() string // DepModuleKeys returns the ModuleKeys representing the dependencies as specified in the buf.lock file. // // Note that ModuleKeys may not have CommitIDs with FileVersionV2. @@ -71,7 +76,7 @@ type BufLockFile interface { // Note that digests are lazily-loaded; if you need to ensure that all digests are valid, run // ValidateBufLockFileDigests(). func NewBufLockFile(fileVersion FileVersion, depModuleKeys []bufmodule.ModuleKey) (BufLockFile, error) { - return newBufLockFile(fileVersion, depModuleKeys) + return newBufLockFile(fileVersion, "", depModuleKeys) } // GetBufLockFileForPrefix gets the buf.lock file at the given bucket prefix. @@ -116,24 +121,33 @@ func PutBufLockFileForPrefix( // // Note that digests are lazily-loaded; if you need to ensure that all digests are valid, run // ValidateFileDigests(). -func ReadBufLockFile(reader io.Reader) (BufLockFile, error) { - return readFile(reader, "lock file", readBufLockFile) +func ReadBufLockFile(reader io.Reader, fileName string) (BufLockFile, error) { + if fileName == "" { + fileName = "lock file" + } + return readFile(reader, fileName, readBufLockFile) } // WriteBufLockFile writes the BufLockFile to the io.Writer. func WriteBufLockFile(writer io.Writer, bufLockFile BufLockFile) error { - return writeFile(writer, "lock file", bufLockFile, writeBufLockFile) + fileIdentifier := bufLockFile.FileName() + if fileIdentifier == "" { + fileIdentifier = "lock file" + } + return writeFile(writer, fileIdentifier, bufLockFile, writeBufLockFile) } // *** PRIVATE *** type bufLockFile struct { fileVersion FileVersion + fileName string depModuleKeys []bufmodule.ModuleKey } func newBufLockFile( fileVersion FileVersion, + fileName string, depModuleKeys []bufmodule.ModuleKey, ) (*bufLockFile, error) { if err := validateNoDuplicateModuleKeysByModuleFullName(depModuleKeys); err != nil { @@ -161,6 +175,10 @@ func (l *bufLockFile) FileVersion() FileVersion { return l.fileVersion } +func (l *bufLockFile) FileName() string { + return l.fileName +} + func (l *bufLockFile) DepModuleKeys() []bufmodule.ModuleKey { return l.depModuleKeys } @@ -170,6 +188,7 @@ func (*bufLockFile) isFile() {} func readBufLockFile( reader io.Reader, + fileName string, allowJSON bool, ) (BufLockFile, error) { data, err := io.ReadAll(reader) @@ -230,7 +249,7 @@ func readBufLockFile( } depModuleKeys[i] = depModuleKey } - return newBufLockFile(fileVersion, depModuleKeys) + return newBufLockFile(fileVersion, fileName, depModuleKeys) case FileVersionV2: var externalBufLockFile externalBufLockFileV2 if err := getUnmarshalStrict(allowJSON)(data, &externalBufLockFile); err != nil { @@ -266,7 +285,7 @@ func readBufLockFile( } depModuleKeys[i] = depModuleKey } - return newBufLockFile(fileVersion, depModuleKeys) + return newBufLockFile(fileVersion, fileName, depModuleKeys) default: // This is a system error since we've already parsed. return nil, syserror.Newf("unknown FileVersion: %v", fileVersion) diff --git a/private/bufpkg/bufconfig/buf_work_yaml_file.go b/private/bufpkg/bufconfig/buf_work_yaml_file.go index 009a677459..d67e213cfb 100644 --- a/private/bufpkg/bufconfig/buf_work_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_work_yaml_file.go @@ -44,6 +44,11 @@ var ( type BufWorkYAMLFile interface { File + // FileName returns the name of the file when the BufWorkYAMLFile was read. + // + // If there was no file name, this returns empty. This can happen when the BufWorkYAMLFile + // is created via ReadBufWorkYAMLFile, for example. + FileName() string // DirPaths returns all the directory paths specified in buf.work.yaml, // relative to the directory with buf.work.yaml. The following are guaranteed: // @@ -62,7 +67,7 @@ type BufWorkYAMLFile interface { // NewBufWorkYAMLFile returns a new validated BufWorkYAMLFile. func NewBufWorkYAMLFile(fileVersion FileVersion, dirPaths []string) (BufWorkYAMLFile, error) { - return newBufWorkYAMLFile(fileVersion, dirPaths) + return newBufWorkYAMLFile(fileVersion, "", dirPaths) } // GetBufWorkYAMLFileForPrefix gets the buf.work.yaml file at the given bucket prefix. @@ -101,23 +106,28 @@ func PutBufWorkYAMLFileForPrefix( } // ReadBufWorkYAMLFile reads the buf.work.yaml file from the io.Reader. -func ReadBufWorkYAMLFile(reader io.Reader) (BufWorkYAMLFile, error) { - return readFile(reader, "workspace file", readBufWorkYAMLFile) +func ReadBufWorkYAMLFile(reader io.Reader, fileName string) (BufWorkYAMLFile, error) { + return readFile(reader, fileName, readBufWorkYAMLFile) } // WriteBufWorkYAMLFile writes the buf.work.yaml to the io.Writer. func WriteBufWorkYAMLFile(writer io.Writer, bufWorkYAMLFile BufWorkYAMLFile) error { - return writeFile(writer, "workspace file", bufWorkYAMLFile, writeBufWorkYAMLFile) + fileIdentifier := bufWorkYAMLFile.FileName() + if fileIdentifier == "" { + fileIdentifier = "config file" + } + return writeFile(writer, fileIdentifier, bufWorkYAMLFile, writeBufWorkYAMLFile) } // *** PRIVATE *** type bufWorkYAMLFile struct { fileVersion FileVersion + fileName string dirPaths []string } -func newBufWorkYAMLFile(fileVersion FileVersion, dirPaths []string) (*bufWorkYAMLFile, error) { +func newBufWorkYAMLFile(fileVersion FileVersion, fileName string, dirPaths []string) (*bufWorkYAMLFile, error) { if fileVersion != FileVersionV1 { return nil, newUnsupportedFileVersionError("", fileVersion) } @@ -127,6 +137,7 @@ func newBufWorkYAMLFile(fileVersion FileVersion, dirPaths []string) (*bufWorkYAM } return &bufWorkYAMLFile{ fileVersion: fileVersion, + fileName: fileName, dirPaths: sortedNormalizedDirPaths, }, nil } @@ -135,6 +146,10 @@ func (w *bufWorkYAMLFile) FileVersion() FileVersion { return w.fileVersion } +func (w *bufWorkYAMLFile) FileName() string { + return w.fileName +} + func (w *bufWorkYAMLFile) DirPaths() []string { return slicesext.Copy(w.dirPaths) } @@ -142,7 +157,7 @@ func (w *bufWorkYAMLFile) DirPaths() []string { func (*bufWorkYAMLFile) isBufWorkYAMLFile() {} func (*bufWorkYAMLFile) isFile() {} -func readBufWorkYAMLFile(reader io.Reader, allowJSON bool) (BufWorkYAMLFile, error) { +func readBufWorkYAMLFile(reader io.Reader, fileName string, allowJSON bool) (BufWorkYAMLFile, error) { data, err := io.ReadAll(reader) if err != nil { return nil, err @@ -160,7 +175,7 @@ func readBufWorkYAMLFile(reader io.Reader, allowJSON bool) (BufWorkYAMLFile, err if err := getUnmarshalStrict(allowJSON)(data, &externalBufWorkYAMLFile); err != nil { return nil, fmt.Errorf("invalid as version %v: %w", fileVersion, err) } - return newBufWorkYAMLFile(fileVersion, externalBufWorkYAMLFile.Directories) + return newBufWorkYAMLFile(fileVersion, fileName, externalBufWorkYAMLFile.Directories) } func writeBufWorkYAMLFile(writer io.Writer, bufWorkYAMLFile BufWorkYAMLFile) error { diff --git a/private/bufpkg/bufconfig/buf_yaml_file.go b/private/bufpkg/bufconfig/buf_yaml_file.go index ee138f3af4..3e617dde88 100644 --- a/private/bufpkg/bufconfig/buf_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_yaml_file.go @@ -34,8 +34,11 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) +// DefaultBufYAMLFileName is the buf.yaml default file name. +const DefaultBufYAMLFileName = "buf.yaml" + var ( - bufYAML = newFileName("buf.yaml", FileVersionV1Beta1, FileVersionV1, FileVersionV2) + bufYAML = newFileName(DefaultBufYAMLFileName, FileVersionV1Beta1, FileVersionV1, FileVersionV2) // Originally we thought we were going to move to buf.mod, and had this around for // a while, but then reverted back to buf.yaml. We still need to support buf.mod as // we released with it, however. @@ -47,6 +50,11 @@ var ( type BufYAMLFile interface { File + // FileName returns the name of the file when the BufYAMLFile was read. + // + // If there was no file name, this returns empty. This can happen when the BufYAMLFile + // is created via ReadBufYAMLFile, for example. + FileName() string // ModuleConfigs returns the ModuleConfigs for the File. // // For v1 buf.yaml, this will only have a single ModuleConfig. @@ -74,7 +82,7 @@ func NewBufYAMLFile( moduleConfigs []ModuleConfig, configuredDepModuleRefs []bufmodule.ModuleRef, ) (BufYAMLFile, error) { - return newBufYAMLFile(fileVersion, moduleConfigs, configuredDepModuleRefs) + return newBufYAMLFile(fileVersion, "", moduleConfigs, configuredDepModuleRefs) } // GetBufYAMLFileForPrefix gets the buf.yaml file at the given bucket prefix. @@ -100,6 +108,7 @@ func GetBufYAMLFileForPrefix( // buf.gen.yamls, or anything of the like, and was very concentrated on "because Bazel." func GetBufYAMLFileForOverride(override string) (BufYAMLFile, error) { var data []byte + var fileName string var err error switch filepath.Ext(override) { case ".json", ".yaml", ".yml": @@ -107,10 +116,11 @@ func GetBufYAMLFileForOverride(override string) (BufYAMLFile, error) { if err != nil { return nil, fmt.Errorf("could not read file: %v", err) } + fileName = filepath.Base(fileName) default: data = []byte(override) } - return ReadBufYAMLFile(bytes.NewReader(data)) + return ReadBufYAMLFile(bytes.NewReader(data), fileName) } // GetBufYAMLFileForOverride get the buf.yaml file for either the usually-flag-based override, @@ -152,25 +162,33 @@ func PutBufYAMLFileForPrefix( } // ReadBufYAMLFile reads the BufYAMLFile from the io.Reader. -func ReadBufYAMLFile(reader io.Reader) (BufYAMLFile, error) { - return readFile(reader, "config file", readBufYAMLFile) +// +// fileName may be empty. +func ReadBufYAMLFile(reader io.Reader, fileName string) (BufYAMLFile, error) { + return readFile(reader, fileName, readBufYAMLFile) } // WriteBufYAMLFile writes the BufYAMLFile to the io.Writer. func WriteBufYAMLFile(writer io.Writer, bufYAMLFile BufYAMLFile) error { - return writeFile(writer, "config file", bufYAMLFile, writeBufYAMLFile) + fileIdentifier := bufYAMLFile.FileName() + if fileIdentifier == "" { + fileIdentifier = "config file" + } + return writeFile(writer, fileIdentifier, bufYAMLFile, writeBufYAMLFile) } // *** PRIVATE *** type bufYAMLFile struct { fileVersion FileVersion + fileName string moduleConfigs []ModuleConfig configuredDepModuleRefs []bufmodule.ModuleRef } func newBufYAMLFile( fileVersion FileVersion, + fileName string, moduleConfigs []ModuleConfig, configuredDepModuleRefs []bufmodule.ModuleRef, ) (*bufYAMLFile, error) { @@ -245,7 +263,7 @@ func newBufYAMLFile( configuredDepModuleRefs, func(i int, j int) bool { return configuredDepModuleRefs[i].ModuleFullName().String() < - configuredDepModuleRefs[i].ModuleFullName().String() + configuredDepModuleRefs[j].ModuleFullName().String() }, ) return &bufYAMLFile{ @@ -259,6 +277,10 @@ func (c *bufYAMLFile) FileVersion() FileVersion { return c.fileVersion } +func (c *bufYAMLFile) FileName() string { + return c.fileName +} + func (c *bufYAMLFile) ModuleConfigs() []ModuleConfig { return slicesext.Copy(c.moduleConfigs) } @@ -272,7 +294,7 @@ func (*bufYAMLFile) isFile() {} // TODO: port tests from bufmoduleconfig, buflintconfig, bufbreakingconfig // TODO: We need to validate all paths on ignore, excludes, etc -func readBufYAMLFile(reader io.Reader, allowJSON bool) (BufYAMLFile, error) { +func readBufYAMLFile(reader io.Reader, fileName string, allowJSON bool) (BufYAMLFile, error) { data, err := io.ReadAll(reader) if err != nil { return nil, err @@ -326,6 +348,7 @@ func readBufYAMLFile(reader io.Reader, allowJSON bool) (BufYAMLFile, error) { } return newBufYAMLFile( fileVersion, + fileName, []ModuleConfig{ moduleConfig, }, @@ -415,6 +438,7 @@ func readBufYAMLFile(reader io.Reader, allowJSON bool) (BufYAMLFile, error) { } return newBufYAMLFile( fileVersion, + fileName, moduleConfigs, configuredDepModuleRefs, ) diff --git a/private/bufpkg/bufconfig/file.go b/private/bufpkg/bufconfig/file.go index 2c99bade27..04981cc4b3 100644 --- a/private/bufpkg/bufconfig/file.go +++ b/private/bufpkg/bufconfig/file.go @@ -44,6 +44,7 @@ func getFileForPrefix[F File]( fileNames []*fileName, readFileFunc func( reader io.Reader, + fileName string, allowJSON bool, ) (F, error), ) (F, error) { @@ -57,7 +58,7 @@ func getFileForPrefix[F File]( var f F return f, err } - f, err := readFileFunc(readObjectCloser, false) + f, err := readFileFunc(readObjectCloser, fileName.Name(), false) if err != nil { return f, multierr.Append(newDecodeError(path, err), readObjectCloser.Close()) } @@ -127,15 +128,16 @@ func putFileForPrefix[F File]( func readFile[F File]( reader io.Reader, - fileIdentifier string, + fileName string, readFileFunc func( reader io.Reader, + fileName string, allowJSON bool, ) (F, error), ) (F, error) { - f, err := readFileFunc(reader, true) + f, err := readFileFunc(reader, fileName, true) if err != nil { - return f, newDecodeError(fileIdentifier, err) + return f, newDecodeError(fileName, err) } return f, nil } @@ -179,12 +181,18 @@ func getUnmarshalNonStrict(allowJSON bool) func([]byte, interface{}) error { return encoding.UnmarshalYAMLNonStrict } -func newDecodeError(fileIdentifier string, err error) error { +func newDecodeError(fileName string, err error) error { + if fileName == "" { + fileName = "config file" + } // We intercept PathErrors in buffetch to deal with fixing of paths. - return &fs.PathError{Op: "decode", Path: fileIdentifier, Err: err} + return &fs.PathError{Op: "decode", Path: fileName, Err: err} } -func newEncodeError(fileIdentifier string, err error) error { +func newEncodeError(fileName string, err error) error { + if fileName == "" { + fileName = "config file" + } // We intercept PathErrors in buffetch to deal with fixing of paths. - return &fs.PathError{Op: "encode", Path: fileIdentifier, Err: err} + return &fs.PathError{Op: "encode", Path: fileName, Err: err} } From 2f2836f2a8ca7f9d39567a23abf38857334e1cec Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 14:08:09 -0500 Subject: [PATCH 05/29] file name is empty unless caller specifies non-empty file name --- private/bufpkg/bufconfig/buf_lock_file.go | 9 +-------- private/bufpkg/bufconfig/buf_work_yaml_file.go | 6 +----- private/bufpkg/bufconfig/buf_yaml_file.go | 6 +----- private/bufpkg/bufconfig/file.go | 5 ++++- 4 files changed, 7 insertions(+), 19 deletions(-) diff --git a/private/bufpkg/bufconfig/buf_lock_file.go b/private/bufpkg/bufconfig/buf_lock_file.go index 6deaa98777..145018dfa7 100644 --- a/private/bufpkg/bufconfig/buf_lock_file.go +++ b/private/bufpkg/bufconfig/buf_lock_file.go @@ -122,19 +122,12 @@ func PutBufLockFileForPrefix( // Note that digests are lazily-loaded; if you need to ensure that all digests are valid, run // ValidateFileDigests(). func ReadBufLockFile(reader io.Reader, fileName string) (BufLockFile, error) { - if fileName == "" { - fileName = "lock file" - } return readFile(reader, fileName, readBufLockFile) } // WriteBufLockFile writes the BufLockFile to the io.Writer. func WriteBufLockFile(writer io.Writer, bufLockFile BufLockFile) error { - fileIdentifier := bufLockFile.FileName() - if fileIdentifier == "" { - fileIdentifier = "lock file" - } - return writeFile(writer, fileIdentifier, bufLockFile, writeBufLockFile) + return writeFile(writer, bufLockFile.FileName(), bufLockFile, writeBufLockFile) } // *** PRIVATE *** diff --git a/private/bufpkg/bufconfig/buf_work_yaml_file.go b/private/bufpkg/bufconfig/buf_work_yaml_file.go index d67e213cfb..eb2b900a9f 100644 --- a/private/bufpkg/bufconfig/buf_work_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_work_yaml_file.go @@ -112,11 +112,7 @@ func ReadBufWorkYAMLFile(reader io.Reader, fileName string) (BufWorkYAMLFile, er // WriteBufWorkYAMLFile writes the buf.work.yaml to the io.Writer. func WriteBufWorkYAMLFile(writer io.Writer, bufWorkYAMLFile BufWorkYAMLFile) error { - fileIdentifier := bufWorkYAMLFile.FileName() - if fileIdentifier == "" { - fileIdentifier = "config file" - } - return writeFile(writer, fileIdentifier, bufWorkYAMLFile, writeBufWorkYAMLFile) + return writeFile(writer, bufWorkYAMLFile.FileName(), bufWorkYAMLFile, writeBufWorkYAMLFile) } // *** PRIVATE *** diff --git a/private/bufpkg/bufconfig/buf_yaml_file.go b/private/bufpkg/bufconfig/buf_yaml_file.go index 3e617dde88..fd1a0ee852 100644 --- a/private/bufpkg/bufconfig/buf_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_yaml_file.go @@ -170,11 +170,7 @@ func ReadBufYAMLFile(reader io.Reader, fileName string) (BufYAMLFile, error) { // WriteBufYAMLFile writes the BufYAMLFile to the io.Writer. func WriteBufYAMLFile(writer io.Writer, bufYAMLFile BufYAMLFile) error { - fileIdentifier := bufYAMLFile.FileName() - if fileIdentifier == "" { - fileIdentifier = "config file" - } - return writeFile(writer, fileIdentifier, bufYAMLFile, writeBufYAMLFile) + return writeFile(writer, bufYAMLFile.FileName(), bufYAMLFile, writeBufYAMLFile) } // *** PRIVATE *** diff --git a/private/bufpkg/bufconfig/file.go b/private/bufpkg/bufconfig/file.go index 04981cc4b3..91a5f2caf9 100644 --- a/private/bufpkg/bufconfig/file.go +++ b/private/bufpkg/bufconfig/file.go @@ -144,13 +144,16 @@ func readFile[F File]( func writeFile[F File]( writer io.Writer, - fileIdentifier string, + fileName string, f F, writeFileFunc func( writer io.Writer, f F, ) error, ) error { + if err := writeFileFunc(writer, f); err != nil { + return newDecodeError(fileName, err) + } return writeFileFunc(writer, f) } From 3f7d3c97e274ca997db6158c3f8b395bc12cd6b7 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 14:15:14 -0500 Subject: [PATCH 06/29] update comments --- private/bufpkg/bufconfig/buf_lock_file.go | 2 ++ private/bufpkg/bufconfig/buf_work_yaml_file.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/private/bufpkg/bufconfig/buf_lock_file.go b/private/bufpkg/bufconfig/buf_lock_file.go index 145018dfa7..a54d30ff91 100644 --- a/private/bufpkg/bufconfig/buf_lock_file.go +++ b/private/bufpkg/bufconfig/buf_lock_file.go @@ -121,6 +121,8 @@ func PutBufLockFileForPrefix( // // Note that digests are lazily-loaded; if you need to ensure that all digests are valid, run // ValidateFileDigests(). +// +// fileName may be empty. func ReadBufLockFile(reader io.Reader, fileName string) (BufLockFile, error) { return readFile(reader, fileName, readBufLockFile) } diff --git a/private/bufpkg/bufconfig/buf_work_yaml_file.go b/private/bufpkg/bufconfig/buf_work_yaml_file.go index eb2b900a9f..e4ce909aed 100644 --- a/private/bufpkg/bufconfig/buf_work_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_work_yaml_file.go @@ -106,6 +106,8 @@ func PutBufWorkYAMLFileForPrefix( } // ReadBufWorkYAMLFile reads the buf.work.yaml file from the io.Reader. +// +// fileName may be empty. func ReadBufWorkYAMLFile(reader io.Reader, fileName string) (BufWorkYAMLFile, error) { return readFile(reader, fileName, readBufWorkYAMLFile) } From 24634df726d8efe576044a1c74f5405153618302 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 14:18:15 -0500 Subject: [PATCH 07/29] comment --- private/bufpkg/bufconfig/buf_lock_file.go | 5 ++++- private/bufpkg/bufconfig/buf_work_yaml_file.go | 5 ++++- private/bufpkg/bufconfig/buf_yaml_file.go | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/private/bufpkg/bufconfig/buf_lock_file.go b/private/bufpkg/bufconfig/buf_lock_file.go index a54d30ff91..e77cefc10d 100644 --- a/private/bufpkg/bufconfig/buf_lock_file.go +++ b/private/bufpkg/bufconfig/buf_lock_file.go @@ -31,11 +31,14 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) +// DefaultBufLockFileName is default buf.lock file name. +const DefaultBufLockFileName = "buf.lock" + var ( // bufLockFileHeader is the header prepended to any lock files. bufLockFileHeader = []byte("# Generated by buf. DO NOT EDIT.\n") - bufLock = newFileName("buf.lock", FileVersionV1Beta1, FileVersionV1, FileVersionV2) + bufLock = newFileName(DefaultBufLockFileName, FileVersionV1Beta1, FileVersionV1, FileVersionV2) bufLockFileNames = []*fileName{bufLock} deprecatedDigestTypeToPrefix = map[string]string{ diff --git a/private/bufpkg/bufconfig/buf_work_yaml_file.go b/private/bufpkg/bufconfig/buf_work_yaml_file.go index e4ce909aed..067bb4a507 100644 --- a/private/bufpkg/bufconfig/buf_work_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_work_yaml_file.go @@ -27,9 +27,12 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) +// DefaultBufWorkYAMLFileName is the default buf.work.yaml file name. +const DefaultBufWorkYAMLFileName = "buf.work.yaml" + var ( // We only supported buf.work.yamls in v1. - bufWorkYAML = newFileName("buf.work.yaml", FileVersionV1) + bufWorkYAML = newFileName(DefaultBufWorkYAMLFileName, FileVersionV1) // Originally we thought we were going to move to buf.work, and had this around for // a while, but then reverted back to buf.work.yaml. We still need to support buf.work as // we released with it, however. diff --git a/private/bufpkg/bufconfig/buf_yaml_file.go b/private/bufpkg/bufconfig/buf_yaml_file.go index fd1a0ee852..c2c8f8f283 100644 --- a/private/bufpkg/bufconfig/buf_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_yaml_file.go @@ -34,7 +34,7 @@ import ( "github.com/bufbuild/buf/private/pkg/syserror" ) -// DefaultBufYAMLFileName is the buf.yaml default file name. +// DefaultBufYAMLFileName is the default buf.yaml file name. const DefaultBufYAMLFileName = "buf.yaml" var ( From 23ed5a58c3047112a957823d52bfe2a24d8a5233 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 15:00:30 -0500 Subject: [PATCH 08/29] commit --- private/buf/bufmigrate/migrator.go | 82 +++++++++-------------- private/buf/cmd/buf/buf.go | 2 +- private/bufpkg/bufconfig/buf_yaml_file.go | 2 +- 3 files changed, 32 insertions(+), 54 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 8c05f57922..e5d0703b0c 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -38,7 +38,6 @@ import ( "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/syserror" - "go.uber.org/multierr" "go.uber.org/zap" ) @@ -80,31 +79,18 @@ func (m *migrator) addWorkspace( ctx context.Context, workspaceDirectory string, ) (retErr error) { - bufWorkYAMLPath, err := findFirstExistingPath( - filepath.Join(workspaceDirectory, bufconfig.DefaultBufWorkYAMLFileName), - filepath.Join(workspaceDirectory, bufconfig.LegacyBufWorkYAMLFileName), + bufWorkYAML, err := bufconfig.GetBufWorkYAMLFileForPrefix( + ctx, + m.rootBucket, + workspaceDirectory, ) if errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf( - "workspace %q does not have a %s or %s", - workspaceDirectory, - bufconfig.DefaultBufWorkYAMLFileName, - bufconfig.LegacyBufWorkYAMLFileName, - ) - } - if err != nil { - return err - } - file, err := os.Open(bufWorkYAMLPath) - // Not checking if err is fs.ErrNotExist because we just did that. - if err != nil { - return err + return fmt.Errorf("%q does not have a workspace configuration file", workspaceDirectory) } - bufWorkYAML, err := bufconfig.ReadBufWorkYAMLFile(file) if err != nil { return err } - m.seenFiles[bufWorkYAMLPath] = struct{}{} + m.seenFiles[filepath.Join(workspaceDirectory, bufWorkYAML.FileName())] = struct{}{} for _, moduleDirRelativeToWorkspace := range bufWorkYAML.DirPaths() { if err := m.addModuleDirectory(ctx, filepath.Join(workspaceDirectory, moduleDirRelativeToWorkspace)); err != nil { return err @@ -119,9 +105,10 @@ func (m *migrator) addModuleDirectory( // moduleDir is the relative path (relative to ".") to the module directory moduleDir string, ) (retErr error) { - bufYAMLPath, err := findFirstExistingPath( - filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName), - filepath.Join(moduleDir, bufconfig.LegacyBufYAMLFileName), + bufYAML, err := bufconfig.GetBufYAMLFileForPrefix( + ctx, + m.rootBucket, + moduleDir, ) if errors.Is(errors.Unwrap(err), fs.ErrNotExist) { moduleDirInMigratedBufYAML, err := filepath.Rel(m.destinationDir, moduleDir) @@ -150,18 +137,7 @@ func (m *migrator) addModuleDirectory( if err != nil { return err } - file, err := os.Open(bufYAMLPath) - // Not checking if err is fs.ErrNotExist because we just did that. - if err != nil { - return err - } - defer func() { - retErr = multierr.Append(retErr, file.Close()) - }() - bufYAML, err := bufconfig.ReadBufYAMLFile(file) - if err != nil { - return err - } + bufYAMLPath := filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName) if _, ok := m.seenFiles[bufYAMLPath]; ok { return nil } @@ -426,9 +402,25 @@ func (m *migrator) buildBufYAMLAndBufLock( return !ok }, ) - // TODO: enable this when ready - moduleRefs := filteredModuleDependencies - var moduleKeys []bufmodule.ModuleKey + // TODO: the next two variables have placeholder values. Inside the if-statement + // is what should happen. + moduleRefs := slicesext.MapValuesToSlice( + slicesext.ToValuesMap( + filteredModuleDependencies, + func(moduleRef bufmodule.ModuleRef) string { + return moduleRef.ModuleFullName().String() + }, + ), + ) + moduleKeys := slicesext.MapValuesToSlice( + slicesext.ToValuesMap( + m.depModuleKeys, + func(moduleKey bufmodule.ModuleKey) string { + return moduleKey.ModuleFullName().String() + }, + ), + ) + // TODO: use this logic when it's tested and when commit service is ready. if false { moduleToRefToCommit, err := getModuleToRefToCommit(ctx, filteredModuleDependencies, m.clientProvider) if err != nil { @@ -482,20 +474,6 @@ func (m *migrator) filesToDelete() []string { return slicesext.MapKeysToSortedSlice(m.seenFiles) } -func findFirstExistingPath(paths ...string) (string, error) { - for _, path := range paths { - _, err := os.Stat(path) - if errors.Is(err, fs.ErrNotExist) { - continue - } - if err != nil { - return "", err - } - return path, nil - } - return "", fs.ErrNotExist -} - func resolvedDeclaredAndLockedDependencies( moduleToRefToCommit map[string]map[string]*modulev1beta1.Commit, commitIDToCommit map[string]*modulev1beta1.Commit, diff --git a/private/buf/cmd/buf/buf.go b/private/buf/cmd/buf/buf.go index 1b43ed1b04..385c12643b 100644 --- a/private/buf/cmd/buf/buf.go +++ b/private/buf/cmd/buf/buf.go @@ -141,7 +141,7 @@ func NewRootCommand(name string) *appcmd.Command { Short: "Beta commands. Unstable and likely to change", SubCommands: []*appcmd.Command{ graph.NewCommand("graph", builder), - migrate.NewCommand("migrate-v1", builder), + migrate.NewCommand("migrate", builder), price.NewCommand("price", builder), stats.NewCommand("stats", builder), studioagent.NewCommand("studio-agent", builder), diff --git a/private/bufpkg/bufconfig/buf_yaml_file.go b/private/bufpkg/bufconfig/buf_yaml_file.go index c733d453cc..c2c8f8f283 100644 --- a/private/bufpkg/bufconfig/buf_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_yaml_file.go @@ -42,7 +42,7 @@ var ( // Originally we thought we were going to move to buf.mod, and had this around for // a while, but then reverted back to buf.yaml. We still need to support buf.mod as // we released with it, however. - bufMod = newFileName(LegacyBufYAMLFileName, FileVersionV1Beta1, FileVersionV1) + bufMod = newFileName("buf.mod", FileVersionV1Beta1, FileVersionV1) bufYAMLFileNames = []*fileName{bufYAML, bufMod} ) From 0305eaf27f494e4aadd6d06682c4144fdccb9f82 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 15:01:14 -0500 Subject: [PATCH 09/29] fix --- private/bufpkg/bufconfig/file.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private/bufpkg/bufconfig/file.go b/private/bufpkg/bufconfig/file.go index 91a5f2caf9..e3703468c3 100644 --- a/private/bufpkg/bufconfig/file.go +++ b/private/bufpkg/bufconfig/file.go @@ -154,7 +154,7 @@ func writeFile[F File]( if err := writeFileFunc(writer, f); err != nil { return newDecodeError(fileName, err) } - return writeFileFunc(writer, f) + return nil } func getFileVersionForData( From 2a6f3cfe96a33d392f9f7ed98883efa68dacdebc Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Tue, 12 Dec 2023 17:46:39 -0500 Subject: [PATCH 10/29] migrate generation --- private/buf/bufmigrate/bufmigrate.go | 10 +- private/buf/bufmigrate/migrator.go | 203 +++++++++++------- .../cmd/buf/command/beta/migrate/migrate.go | 4 + .../buf/cmd/buf/command/generate/generate.go | 46 ---- 4 files changed, 141 insertions(+), 122 deletions(-) diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index d73ce05b3f..ece635f129 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -39,9 +39,8 @@ func Migrate( for _, option := range options { option(migrateOptions) } - // TODO: check buf.gen.yaml as well when it's added - if migrateOptions.workspaceDirectory == "" && len(migrateOptions.moduleDirectories) == 0 { - return errors.New("no ") + if migrateOptions.workspaceDirectory == "" && len(migrateOptions.moduleDirectories) == 0 && len(migrateOptions.bufGenYAMLPaths) == 0 { + return errors.New("no directory or file specified") } bucket, err := storageProvider.NewReadWriteBucket( ".", @@ -75,6 +74,11 @@ func Migrate( return err } } + for _, bufGenYAMLPath := range migrateOptions.bufGenYAMLPaths { + if err := migrator.addBufGenYAML(bufGenYAMLPath); err != nil { + return err + } + } if migrateOptions.dryRun { return migrator.migrateAsDryRun(ctx, migrateOptions.dryRunWriter) } diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index e5d0703b0c..40c8292520 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -38,6 +38,7 @@ import ( "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/syserror" + "go.uber.org/multierr" "go.uber.org/zap" ) @@ -50,13 +51,12 @@ type migrator struct { // the bucket at "." rootBucket storage.ReadWriteBucket - // useful for creating new files - moduleConfigs []bufconfig.ModuleConfig - moduleDependencies []bufmodule.ModuleRef - depModuleKeys []bufmodule.ModuleKey - - moduleNameToParentFile map[string]string - seenFiles map[string]struct{} + moduleConfigs []bufconfig.ModuleConfig + moduleDependencies []bufmodule.ModuleRef + depModuleKeys []bufmodule.ModuleKey + pathToMigratedBufGenYAML map[string]bufconfig.BufGenYAMLFile + moduleNameToParentFile map[string]string + filesToDelete map[string]struct{} } func newMigrator( @@ -66,13 +66,47 @@ func newMigrator( destinationDir string, ) *migrator { return &migrator{ - logger: logger, - clientProvider: clientProvider, - destinationDir: destinationDir, - rootBucket: rootBucket, - moduleNameToParentFile: map[string]string{}, - seenFiles: map[string]struct{}{}, + logger: logger, + clientProvider: clientProvider, + destinationDir: destinationDir, + rootBucket: rootBucket, + pathToMigratedBufGenYAML: map[string]bufconfig.BufGenYAMLFile{}, + moduleNameToParentFile: map[string]string{}, + filesToDelete: map[string]struct{}{}, + } +} + +func (m *migrator) addBufGenYAML( + bufGenYAMLPath string, +) (retErr error) { + file, err := os.Open(bufGenYAMLPath) + if err != nil { + return err + } + defer func() { + retErr = multierr.Append(retErr, file.Close()) + }() + bufGenYAML, err := bufconfig.ReadBufGenYAMLFile(file) + if err != nil { + return err + } + if bufGenYAML.FileVersion() == bufconfig.FileVersionV2 { + m.logger.Sugar().Warnf("%s is already in v2", bufGenYAMLPath) + return nil + } + if typeConfig := bufGenYAML.GenerateConfig().GenerateTypeConfig(); typeConfig != nil && len(typeConfig.IncludeTypes()) > 0 { + m.logger.Sugar().Warnf( + "%s has types configuration and is migrated to v2 without it. To add these types your v2 configuration, first add an input and add these types to it.", + ) } + migratedBufGenYAML := bufconfig.NewBufGenYAMLFile( + bufconfig.FileVersionV2, + bufGenYAML.GenerateConfig(), + nil, + ) + m.filesToDelete[bufGenYAMLPath] = struct{}{} + m.pathToMigratedBufGenYAML[bufGenYAMLPath] = migratedBufGenYAML + return nil } func (m *migrator) addWorkspace( @@ -90,7 +124,7 @@ func (m *migrator) addWorkspace( if err != nil { return err } - m.seenFiles[filepath.Join(workspaceDirectory, bufWorkYAML.FileName())] = struct{}{} + m.filesToDelete[filepath.Join(workspaceDirectory, bufWorkYAML.FileName())] = struct{}{} for _, moduleDirRelativeToWorkspace := range bufWorkYAML.DirPaths() { if err := m.addModuleDirectory(ctx, filepath.Join(workspaceDirectory, moduleDirRelativeToWorkspace)); err != nil { return err @@ -138,10 +172,9 @@ func (m *migrator) addModuleDirectory( return err } bufYAMLPath := filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName) - if _, ok := m.seenFiles[bufYAMLPath]; ok { + if _, ok := m.filesToDelete[bufYAMLPath]; ok { return nil } - m.seenFiles[bufYAMLPath] = struct{}{} // TODO: transform paths so that they are relative to the new buf.yaml v2 or module root (depending on buf.yaml v2 semantics) // Paths include RootToExcludes, IgnorePaths, IgnoreIDOrCategoryToPaths switch bufYAML.FileVersion() { @@ -291,10 +324,12 @@ func (m *migrator) addModuleDirectory( } m.moduleDependencies = append(m.moduleDependencies, bufYAML.ConfiguredDepModuleRefs()...) case bufconfig.FileVersionV2: - return fmt.Errorf("%s is already at v2", bufYAMLPath) + m.logger.Sugar().Warnf("%s is already at v2", bufYAMLPath) + return nil default: return syserror.Newf("unexpected version: %v", bufYAML.FileVersion()) } + m.filesToDelete[bufYAMLPath] = struct{}{} bufLockFile, err := bufconfig.GetBufLockFileForPrefix( ctx, m.rootBucket, @@ -310,7 +345,7 @@ func (m *migrator) addModuleDirectory( // We don't need to check whether it's already in the map, but because if it were, // its co-resident buf.yaml would also have been a duplicate and made this // function return at an earlier point. - m.seenFiles[bufLockFilePath] = struct{}{} + m.filesToDelete[bufLockFilePath] = struct{}{} switch bufLockFile.FileVersion() { case bufconfig.FileVersionV1Beta1, bufconfig.FileVersionV1: m.depModuleKeys = append(m.depModuleKeys, bufLockFile.DepModuleKeys()...) @@ -325,68 +360,94 @@ func (m *migrator) addModuleDirectory( func (m *migrator) migrateAsDryRun( ctx context.Context, writer io.Writer, -) error { - migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) - if err != nil { - return err - } - filesToDelete := m.filesToDelete() - var bufYAMLBuffer bytes.Buffer - if err := bufconfig.WriteBufYAMLFile(&bufYAMLBuffer, migratedBufYAML); err != nil { - return err - } - var bufLockBuffer bytes.Buffer - if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { - return err - } +) (retErr error) { fmt.Fprintf( writer, - `in an actual run, these files will be removed: -%s - -these files will be written: -%s: -%s -%s: -%s -`, - strings.Join(filesToDelete, "\n"), - filepath.Join(m.destinationDir, bufconfig.DefaultBufYAMLFileName), - bufYAMLBuffer.String(), - filepath.Join(m.destinationDir, bufconfig.DefaultBufLockFileName), - bufLockBuffer.String(), + "In an actual run, these files will be removed:\n%s\n\nThe following files will be overwritten or created:\n\n", + strings.Join(slicesext.MapKeysToSortedSlice(m.filesToDelete), "\n"), ) + if len(m.moduleConfigs) > 0 { + migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) + if err != nil { + return err + } + var bufYAMLBuffer bytes.Buffer + if err := bufconfig.WriteBufYAMLFile(&bufYAMLBuffer, migratedBufYAML); err != nil { + return err + } + fmt.Fprintf( + writer, + "%s:\n%s\n", + filepath.Join(m.destinationDir, bufconfig.DefaultBufYAMLFileName), + bufYAMLBuffer.String(), + ) + var bufLockBuffer bytes.Buffer + if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { + return err + } + fmt.Fprintf( + writer, + "%s:\n%s\n", + filepath.Join(m.destinationDir, bufconfig.DefaultBufLockFileName), + bufLockBuffer.String(), + ) + } + for bufGenYAMLPath, migratedBufGenYAML := range m.pathToMigratedBufGenYAML { + var bufGenYAMLBuffer bytes.Buffer + if err := bufconfig.WriteBufGenYAMLFile(&bufGenYAMLBuffer, migratedBufGenYAML); err != nil { + return err + } + fmt.Fprintf( + writer, + "%s will be written:\n%s\n", + bufGenYAMLPath, + bufGenYAMLBuffer.String(), + ) + } return nil } func (m *migrator) migrate( ctx context.Context, -) error { - migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) - if err != nil { - return err - } - filesToDelete := m.filesToDelete() - for _, fileToDelete := range filesToDelete { - if err := os.Remove(fileToDelete); err != nil { +) (retErr error) { + for bufGenYAMLPath, migratedBufGenYAML := range m.pathToMigratedBufGenYAML { + file, err := os.OpenFile(bufGenYAMLPath, os.O_WRONLY|os.O_TRUNC, 0644) + if err != nil { + return err + } + defer func() { + retErr = multierr.Append(retErr, file.Close()) + }() + if err := bufconfig.WriteBufGenYAMLFile(file, migratedBufGenYAML); err != nil { return err } } - if err := bufconfig.PutBufYAMLFileForPrefix( - ctx, - m.rootBucket, - m.destinationDir, - migratedBufYAML, - ); err != nil { - return err - } - if err := bufconfig.PutBufLockFileForPrefix( - ctx, - m.rootBucket, - m.destinationDir, - migratedBufLock, - ); err != nil { - return err + if len(m.moduleConfigs) > 0 { + migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) + if err != nil { + return err + } + for _, fileToDelete := range slicesext.MapKeysToSortedSlice(m.filesToDelete) { + if err := os.Remove(fileToDelete); err != nil { + return err + } + } + if err := bufconfig.PutBufYAMLFileForPrefix( + ctx, + m.rootBucket, + m.destinationDir, + migratedBufYAML, + ); err != nil { + return err + } + if err := bufconfig.PutBufLockFileForPrefix( + ctx, + m.rootBucket, + m.destinationDir, + migratedBufLock, + ); err != nil { + return err + } } return nil } @@ -470,10 +531,6 @@ func (m *migrator) appendModuleConfig(moduleConfig bufconfig.ModuleConfig, paren return nil } -func (m *migrator) filesToDelete() []string { - return slicesext.MapKeysToSortedSlice(m.seenFiles) -} - func resolvedDeclaredAndLockedDependencies( moduleToRefToCommit map[string]map[string]*modulev1beta1.Commit, commitIDToCommit map[string]*modulev1beta1.Commit, diff --git a/private/buf/cmd/buf/command/beta/migrate/migrate.go b/private/buf/cmd/buf/command/beta/migrate/migrate.go index e0a6aa5056..51274b8567 100644 --- a/private/buf/cmd/buf/command/beta/migrate/migrate.go +++ b/private/buf/cmd/buf/command/beta/migrate/migrate.go @@ -16,6 +16,7 @@ package migrate import ( "context" + "errors" "github.com/bufbuild/buf/private/buf/bufcli" "github.com/bufbuild/buf/private/buf/bufmigrate" @@ -117,6 +118,9 @@ func run( bufmigrate.MigrateGenerationTemplates(flags.BufGenYAMLPath), ) } + if len(migrateOptions) == 0 { + return errors.New("no directories or files specified") + } if flags.DryRun { migrateOptions = append(migrateOptions, bufmigrate.MigrateAsDryRun(container.Stdout())) } diff --git a/private/buf/cmd/buf/command/generate/generate.go b/private/buf/cmd/buf/command/generate/generate.go index 1d12856415..dcbd46a87e 100644 --- a/private/buf/cmd/buf/command/generate/generate.go +++ b/private/buf/cmd/buf/command/generate/generate.go @@ -53,7 +53,6 @@ const ( disableSymlinksFlagName = "disable-symlinks" typeFlagName = "type" typeDeprecatedFlagName = "include-types" - migrateFlagName = "migrate" ) // NewCommand returns a new Command. @@ -214,7 +213,6 @@ type flags struct { // want to find out what will break if we do. Types []string TypesDeprecated []string - Migrate bool // special InputHashtag string } @@ -283,12 +281,6 @@ func (f *flags) Bind(flagSet *pflag.FlagSet) { nil, "The types (package, message, enum, extension, service, method) that should be included in this image. When specified, the resulting image will only include descriptors to describe the requested types. Flag usage overrides buf.gen.yaml", ) - flagSet.BoolVar( - &f.Migrate, - migrateFlagName, - false, - "Migrate the generation template to the latest version", - ) _ = flagSet.MarkDeprecated(typeDeprecatedFlagName, fmt.Sprintf("Use --%s instead", typeFlagName)) _ = flagSet.MarkHidden(typeDeprecatedFlagName) } @@ -345,22 +337,6 @@ func run( if err != nil { return err } - if flags.Migrate && bufGenYAMLFile.FileVersion() != bufconfig.FileVersionV2 { - migratedBufGenYAMLFile, err := getBufGenYAMLFileWithFlagEquivalence( - ctx, - logger, - bufGenYAMLFile, - input, - *flags, - ) - if err != nil { - return err - } - if err := bufconfig.PutBufGenYAMLFileForPrefix(ctx, bucket, ".", migratedBufGenYAMLFile); err != nil { - return err - } - // TODO: perhaps print a message - } case templatePathExtension == ".yaml" || templatePathExtension == ".yml" || templatePathExtension == ".json": // We should not read from a bucket at "." because this path can jump context. configFile, err := os.Open(flags.Template) @@ -371,33 +347,11 @@ func run( if err != nil { return err } - if flags.Migrate && bufGenYAMLFile.FileVersion() != bufconfig.FileVersionV2 { - migratedBufGenYAMLFile, err := getBufGenYAMLFileWithFlagEquivalence( - ctx, - logger, - bufGenYAMLFile, - input, - *flags, - ) - if err != nil { - return err - } - if err := bufconfig.WriteBufGenYAMLFile(configFile, migratedBufGenYAMLFile); err != nil { - return err - } - // TODO: perhaps print a message - } default: bufGenYAMLFile, err = bufconfig.ReadBufGenYAMLFile(strings.NewReader(flags.Template)) if err != nil { return err } - if flags.Migrate && bufGenYAMLFile.FileVersion() != bufconfig.FileVersionV2 { - return fmt.Errorf( - "invalid template: %q, migration can only apply to a file on disk with extension .yaml, .yml or .json", - flags.Template, - ) - } } images, err := getInputImages( ctx, From 66183e7b903cd71bfd0f6d9b85eb8bf31ed9b04a Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Wed, 13 Dec 2023 16:51:34 -0500 Subject: [PATCH 11/29] update resolve --- private/buf/bufmigrate/migrator.go | 206 +++++++++++++++++------------ 1 file changed, 120 insertions(+), 86 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 40c8292520..c1c20d681e 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -455,51 +455,120 @@ func (m *migrator) migrate( func (m *migrator) buildBufYAMLAndBufLock( ctx context.Context, ) (bufconfig.BufYAMLFile, bufconfig.BufLockFile, error) { - // Remove declared dependencies that are also modules in this workspace. - filteredModuleDependencies := slicesext.Filter( - m.moduleDependencies, - func(moduleRef bufmodule.ModuleRef) bool { - _, ok := m.moduleNameToParentFile[moduleRef.ModuleFullName().String()] - return !ok - }, - ) - // TODO: the next two variables have placeholder values. Inside the if-statement - // is what should happen. - moduleRefs := slicesext.MapValuesToSlice( - slicesext.ToValuesMap( - filteredModuleDependencies, - func(moduleRef bufmodule.ModuleRef) string { - return moduleRef.ModuleFullName().String() - }, - ), - ) - moduleKeys := slicesext.MapValuesToSlice( - slicesext.ToValuesMap( - m.depModuleKeys, + depModuleToRefs := make(map[string][]bufmodule.ModuleRef) + for _, depModuleRef := range m.moduleDependencies { + moduleFullName := depModuleRef.ModuleFullName().String() + if _, ok := m.moduleNameToParentFile[moduleFullName]; !ok { + continue + } + depModuleToRefs[moduleFullName] = append(depModuleToRefs[moduleFullName], depModuleRef) + } + depModuleToKeys := make(map[string][]bufmodule.ModuleKey) + for _, depModuleKey := range m.depModuleKeys { + moduleFullName := depModuleKey.ModuleFullName().String() + // We are only removing lock entries that are in the workspace. If a lock + // entry is not in the workspace, it could be an indirect + // A lock entry could be for an indirect dependenceny not listed in deps in buf.yaml. + depModuleToKeys[moduleFullName] = append(depModuleToKeys[moduleFullName], depModuleKey) + } + areDependenciesResolved := true + for depModule, depModuleRefs := range depModuleToRefs { + refStringToRef := make(map[string]bufmodule.ModuleRef) + for _, ref := range depModuleRefs { + // Add ref even if ref.Ref() is empty. Therefore, slicesext.ToValuesMap is not used. + refStringToRef[ref.Ref()] = ref + } + // If there are both buf.build/foo/bar and buf.build/foo/bar:some_ref, take the latter. + if len(refStringToRef) > 1 { + delete(refStringToRef, "") + } + if len(refStringToRef) > 1 { + areDependenciesResolved = false + } + depModuleToRefs[depModule] = slicesext.MapValuesToSlice(refStringToRef) + } + for depModule, depModuleKeys := range depModuleToKeys { + commitIDToKey := slicesext.ToValuesMap( + depModuleKeys, func(moduleKey bufmodule.ModuleKey) string { - return moduleKey.ModuleFullName().String() + return moduleKey.CommitID() }, - ), - ) - // TODO: use this logic when it's tested and when commit service is ready. - if false { - moduleToRefToCommit, err := getModuleToRefToCommit(ctx, filteredModuleDependencies, m.clientProvider) + ) + if len(commitIDToKey) > 1 { + areDependenciesResolved = false + } + depModuleToKeys[depModule] = slicesext.MapValuesToSlice(commitIDToKey) + } + if areDependenciesResolved { + resolvedDepModuleRefs := make([]bufmodule.ModuleRef, 0, len(depModuleToRefs)) + for _, depModuleRefs := range depModuleToRefs { + // depModuleRefs is guaranteed to have length 1 + resolvedDepModuleRefs = append(resolvedDepModuleRefs, depModuleRefs...) + } + bufYAML, err := bufconfig.NewBufYAMLFile( + bufconfig.FileVersionV2, + m.moduleConfigs, + resolvedDepModuleRefs, + ) + if err != nil { + return nil, nil, err + } + resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleToKeys)) + for _, depModuleKeys := range depModuleToKeys { + resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys...) + } + bufLock, err := bufconfig.NewBufLockFile( + bufconfig.FileVersionV2, + resolvedDepModuleKeys, + ) if err != nil { return nil, nil, err } - commitIDToCommit, err := getCommitIDToCommit(ctx, m.clientProvider, m.depModuleKeys) + return bufYAML, bufLock, nil + } + if true { + resolvedDepModuleRefs := make([]bufmodule.ModuleRef, 0, len(depModuleToRefs)) + for _, depModuleRefs := range depModuleToRefs { + resolvedDepModuleRefs = append(resolvedDepModuleRefs, depModuleRefs[0]) + } + bufYAML, err := bufconfig.NewBufYAMLFile( + bufconfig.FileVersionV2, + m.moduleConfigs, + resolvedDepModuleRefs, + ) if err != nil { return nil, nil, err } - moduleRefs, moduleKeys, err = resolvedDeclaredAndLockedDependencies( - moduleToRefToCommit, - commitIDToCommit, - filteredModuleDependencies, - m.depModuleKeys, + resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleToKeys)) + for _, depModuleKeys := range depModuleToKeys { + resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys[0]) + } + bufLock, err := bufconfig.NewBufLockFile( + bufconfig.FileVersionV2, + resolvedDepModuleKeys, ) if err != nil { return nil, nil, err } + return bufYAML, bufLock, nil + } + // TODO: the code below this line isn't currently reachable. + moduleToRefToCommit, err := getModuleToRefToCommit(ctx, m.clientProvider, m.moduleDependencies) + if err != nil { + return nil, nil, err + } + commitIDToCommit, err := getCommitIDToCommit(ctx, m.clientProvider, m.depModuleKeys) + if err != nil { + return nil, nil, err + } + moduleRefs, moduleKeys, err := resolvedDeclaredAndLockedDependencies( + moduleToRefToCommit, + commitIDToCommit, + depModuleToRefs, + depModuleToKeys, + ) + if err != nil { + return nil, nil, err } bufYAML, err := bufconfig.NewBufYAMLFile( bufconfig.FileVersionV2, @@ -534,59 +603,24 @@ func (m *migrator) appendModuleConfig(moduleConfig bufconfig.ModuleConfig, paren func resolvedDeclaredAndLockedDependencies( moduleToRefToCommit map[string]map[string]*modulev1beta1.Commit, commitIDToCommit map[string]*modulev1beta1.Commit, - declaredDependencies []bufmodule.ModuleRef, - lockedDependencies []bufmodule.ModuleKey, + moduleFullNameToDeclaredRefs map[string][]bufmodule.ModuleRef, + moduleFullNameToLockKeys map[string][]bufmodule.ModuleKey, ) ([]bufmodule.ModuleRef, []bufmodule.ModuleKey, error) { - depModuleFullNameToRefs := make(map[string][]bufmodule.ModuleRef) - for _, depModuleRef := range declaredDependencies { - moduleFullName := depModuleRef.ModuleFullName().String() - depModuleFullNameToRefs[moduleFullName] = append(depModuleFullNameToRefs[moduleFullName], depModuleRef) - } depModuleFullNameToResolvedRef := make(map[string]bufmodule.ModuleRef) - for moduleFullName, refs := range depModuleFullNameToRefs { - nonEmptyRef := slicesext.Filter( - refs, - func(ref bufmodule.ModuleRef) bool { - return ref.Ref() != "" - }, - ) - switch len(nonEmptyRef) { - case 0: - // All refs are empty, we take the first one (they are all the same). refs is guaranteed not empty, - // by the construction of depModuleFullNameToRefs. - depModuleFullNameToResolvedRef[moduleFullName] = refs[0] - default: - // There are multiple pinned versions of the same dependency, we use the latest one. - sort.Slice(nonEmptyRef, func(i, j int) bool { - refToCommit := moduleToRefToCommit[moduleFullName] - iTime := refToCommit[refs[i].Ref()].GetCreateTime().AsTime() - jTime := refToCommit[refs[j].Ref()].GetCreateTime().AsTime() - return iTime.After(jTime) - }) - depModuleFullNameToResolvedRef[moduleFullName] = nonEmptyRef[0] - } - } - // We only want locked dependencies that correspond to declared dependencies. - lockedDependencies = slicesext.Filter( - lockedDependencies, - func(lockedDependency bufmodule.ModuleKey) bool { - _, ok := depModuleFullNameToRefs[lockedDependency.ModuleFullName().String()] - return ok - }, - ) - depModuleFullNameToModuleKeys := make(map[string][]bufmodule.ModuleKey) - for _, depModuleKey := range lockedDependencies { - depModuleFullName := depModuleKey.ModuleFullName().String() - depModuleFullNameToModuleKeys[depModuleFullName] = append(depModuleFullNameToModuleKeys[depModuleFullName], depModuleKey) + for moduleFullName, refs := range moduleFullNameToDeclaredRefs { + // There are multiple pinned versions of the same dependency, we use the latest one. + sort.Slice(refs, func(i, j int) bool { + refToCommit := moduleToRefToCommit[moduleFullName] + iTime := refToCommit[refs[i].Ref()].GetCreateTime().AsTime() + jTime := refToCommit[refs[j].Ref()].GetCreateTime().AsTime() + return iTime.After(jTime) + }) + depModuleFullNameToResolvedRef[moduleFullName] = refs[0] } - resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleFullNameToModuleKeys)) - for moduleFullName, depModuleKeys := range depModuleFullNameToModuleKeys { + resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(moduleFullNameToLockKeys)) + for moduleFullName, lockKeys := range moduleFullNameToLockKeys { resolvedRef := depModuleFullNameToResolvedRef[moduleFullName] if resolvedRef.Ref() != "" { - // TODO: do we want to do all this? It feels like we are doing a partial `buf mod update`. - // More specifically, it's possible that the declared dependency pin does not have a corresponding - // lock entry. For example, the buf.lock might not exist. - // Might as well do this for all lock entries. resolvedCommit := moduleToRefToCommit[moduleFullName][resolvedRef.Ref()] key, err := bufmodule.NewModuleKey( resolvedRef.ModuleFullName(), @@ -600,12 +634,12 @@ func resolvedDeclaredAndLockedDependencies( continue } // Use the lastest - sort.Slice(depModuleKeys, func(i, j int) bool { - iTime := commitIDToCommit[depModuleKeys[i].CommitID()].GetCreateTime().AsTime() - jTime := commitIDToCommit[depModuleKeys[j].CommitID()].GetCreateTime().AsTime() + sort.Slice(lockKeys, func(i, j int) bool { + iTime := commitIDToCommit[lockKeys[i].CommitID()].GetCreateTime().AsTime() + jTime := commitIDToCommit[lockKeys[j].CommitID()].GetCreateTime().AsTime() return iTime.After(jTime) }) - resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys[0]) + resolvedDepModuleKeys = append(resolvedDepModuleKeys, lockKeys[0]) } resolvedDeclaredDependencies := slicesext.MapValuesToSlice(depModuleFullNameToResolvedRef) sort.Slice(resolvedDeclaredDependencies, func(i, j int) bool { @@ -619,8 +653,8 @@ func resolvedDeclaredAndLockedDependencies( func getModuleToRefToCommit( ctx context.Context, - moduleRefs []bufmodule.ModuleRef, clientProvider bufapi.ClientProvider, + moduleRefs []bufmodule.ModuleRef, ) (map[string]map[string]*modulev1beta1.Commit, error) { moduleToRefToCommit := make(map[string]map[string]*modulev1beta1.Commit) for _, moduleRef := range moduleRefs { From ab1956f9bf0241d56e748976a1dd859e7fd992fd Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Wed, 13 Dec 2023 16:54:49 -0500 Subject: [PATCH 12/29] update flag names --- private/buf/cmd/buf/command/beta/migrate/migrate.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/private/buf/cmd/buf/command/beta/migrate/migrate.go b/private/buf/cmd/buf/command/beta/migrate/migrate.go index 51274b8567..9dbdc24285 100644 --- a/private/buf/cmd/buf/command/beta/migrate/migrate.go +++ b/private/buf/cmd/buf/command/beta/migrate/migrate.go @@ -28,9 +28,9 @@ import ( ) const ( - workspaceDirectoryFlagName = "workspace-dir" - moduleDirectoriesFlagName = "module-dir" - bufGenYAMLPathFlagName = "generate-template" + workspaceDirectoryFlagName = "workspace" + moduleDirectoriesFlagName = "module" + bufGenYAMLPathFlagName = "template" dryRunFlagName = "dry-run" ) From 0d4d31e40de0d30fd0b07a22d7ec3a6589162b12 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Wed, 13 Dec 2023 17:10:54 -0500 Subject: [PATCH 13/29] dont create buf.lock if we haven't seen one --- private/buf/bufmigrate/migrator.go | 92 ++++++++++++++++++------------ 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index c1c20d681e..fdd64cba52 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -45,14 +45,15 @@ import ( type migrator struct { logger *zap.Logger clientProvider bufapi.ClientProvider + // the bucket at "." + rootBucket storage.ReadWriteBucket // the directory where the migrated buf.yaml live, this is useful for computing // module directory paths, and possibly other paths. destinationDir string - // the bucket at "." - rootBucket storage.ReadWriteBucket moduleConfigs []bufconfig.ModuleConfig moduleDependencies []bufmodule.ModuleRef + isLockFileSeen bool depModuleKeys []bufmodule.ModuleKey pathToMigratedBufGenYAML map[string]bufconfig.BufGenYAMLFile moduleNameToParentFile map[string]string @@ -354,6 +355,7 @@ func (m *migrator) addModuleDirectory( default: return syserror.Newf("unrecognized version: %v", bufLockFile.FileVersion()) } + m.isLockFileSeen = true return nil } @@ -381,16 +383,18 @@ func (m *migrator) migrateAsDryRun( filepath.Join(m.destinationDir, bufconfig.DefaultBufYAMLFileName), bufYAMLBuffer.String(), ) - var bufLockBuffer bytes.Buffer - if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { - return err + if migratedBufLock != nil { + var bufLockBuffer bytes.Buffer + if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { + return err + } + fmt.Fprintf( + writer, + "%s:\n%s\n", + filepath.Join(m.destinationDir, bufconfig.DefaultBufLockFileName), + bufLockBuffer.String(), + ) } - fmt.Fprintf( - writer, - "%s:\n%s\n", - filepath.Join(m.destinationDir, bufconfig.DefaultBufLockFileName), - bufLockBuffer.String(), - ) } for bufGenYAMLPath, migratedBufGenYAML := range m.pathToMigratedBufGenYAML { var bufGenYAMLBuffer bytes.Buffer @@ -440,18 +444,21 @@ func (m *migrator) migrate( ); err != nil { return err } - if err := bufconfig.PutBufLockFileForPrefix( - ctx, - m.rootBucket, - m.destinationDir, - migratedBufLock, - ); err != nil { - return err + if migratedBufLock != nil { + if err := bufconfig.PutBufLockFileForPrefix( + ctx, + m.rootBucket, + m.destinationDir, + migratedBufLock, + ); err != nil { + return err + } } } return nil } +// If no error, the BufYAMLFile returned is never nil, but the BufLockFile returne may be nil. func (m *migrator) buildBufYAMLAndBufLock( ctx context.Context, ) (bufconfig.BufYAMLFile, bufconfig.BufLockFile, error) { @@ -517,12 +524,15 @@ func (m *migrator) buildBufYAMLAndBufLock( for _, depModuleKeys := range depModuleToKeys { resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys...) } - bufLock, err := bufconfig.NewBufLockFile( - bufconfig.FileVersionV2, - resolvedDepModuleKeys, - ) - if err != nil { - return nil, nil, err + var bufLock bufconfig.BufLockFile + if m.isLockFileSeen { + bufLock, err = bufconfig.NewBufLockFile( + bufconfig.FileVersionV2, + resolvedDepModuleKeys, + ) + if err != nil { + return nil, nil, err + } } return bufYAML, bufLock, nil } @@ -543,12 +553,15 @@ func (m *migrator) buildBufYAMLAndBufLock( for _, depModuleKeys := range depModuleToKeys { resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys[0]) } - bufLock, err := bufconfig.NewBufLockFile( - bufconfig.FileVersionV2, - resolvedDepModuleKeys, - ) - if err != nil { - return nil, nil, err + var bufLock bufconfig.BufLockFile + if m.isLockFileSeen { + bufLock, err = bufconfig.NewBufLockFile( + bufconfig.FileVersionV2, + resolvedDepModuleKeys, + ) + if err != nil { + return nil, nil, err + } } return bufYAML, bufLock, nil } @@ -561,7 +574,7 @@ func (m *migrator) buildBufYAMLAndBufLock( if err != nil { return nil, nil, err } - moduleRefs, moduleKeys, err := resolvedDeclaredAndLockedDependencies( + resolvedDepModuleRefs, resolvedDepModuleKeys, err := resolvedDeclaredAndLockedDependencies( moduleToRefToCommit, commitIDToCommit, depModuleToRefs, @@ -573,17 +586,20 @@ func (m *migrator) buildBufYAMLAndBufLock( bufYAML, err := bufconfig.NewBufYAMLFile( bufconfig.FileVersionV2, m.moduleConfigs, - moduleRefs, + resolvedDepModuleRefs, ) if err != nil { return nil, nil, err } - bufLock, err := bufconfig.NewBufLockFile( - bufconfig.FileVersionV2, - moduleKeys, - ) - if err != nil { - return nil, nil, err + var bufLock bufconfig.BufLockFile + if m.isLockFileSeen { + bufLock, err = bufconfig.NewBufLockFile( + bufconfig.FileVersionV2, + resolvedDepModuleKeys, + ) + if err != nil { + return nil, nil, err + } } return bufYAML, bufLock, nil } From 72a63d2c3fc7d3857919698c594805b7c6bf72c4 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Wed, 13 Dec 2023 17:11:28 -0500 Subject: [PATCH 14/29] field name --- private/buf/bufmigrate/migrator.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index fdd64cba52..645fba2a16 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -53,7 +53,7 @@ type migrator struct { moduleConfigs []bufconfig.ModuleConfig moduleDependencies []bufmodule.ModuleRef - isLockFileSeen bool + hasSeenBufLock bool depModuleKeys []bufmodule.ModuleKey pathToMigratedBufGenYAML map[string]bufconfig.BufGenYAMLFile moduleNameToParentFile map[string]string @@ -355,7 +355,7 @@ func (m *migrator) addModuleDirectory( default: return syserror.Newf("unrecognized version: %v", bufLockFile.FileVersion()) } - m.isLockFileSeen = true + m.hasSeenBufLock = true return nil } @@ -525,7 +525,7 @@ func (m *migrator) buildBufYAMLAndBufLock( resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys...) } var bufLock bufconfig.BufLockFile - if m.isLockFileSeen { + if m.hasSeenBufLock { bufLock, err = bufconfig.NewBufLockFile( bufconfig.FileVersionV2, resolvedDepModuleKeys, @@ -554,7 +554,7 @@ func (m *migrator) buildBufYAMLAndBufLock( resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys[0]) } var bufLock bufconfig.BufLockFile - if m.isLockFileSeen { + if m.hasSeenBufLock { bufLock, err = bufconfig.NewBufLockFile( bufconfig.FileVersionV2, resolvedDepModuleKeys, @@ -592,7 +592,7 @@ func (m *migrator) buildBufYAMLAndBufLock( return nil, nil, err } var bufLock bufconfig.BufLockFile - if m.isLockFileSeen { + if m.hasSeenBufLock { bufLock, err = bufconfig.NewBufLockFile( bufconfig.FileVersionV2, resolvedDepModuleKeys, From bc6eb652962767e787144d35d40bb5aa5344a080 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Wed, 13 Dec 2023 18:30:40 -0500 Subject: [PATCH 15/29] handle module without a config correctly --- private/buf/bufmigrate/migrator.go | 31 +++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 645fba2a16..7d16a2fc7f 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -153,9 +153,34 @@ func (m *migrator) addModuleDirectory( moduleConfig, err := bufconfig.NewModuleConfig( moduleDirInMigratedBufYAML, nil, - nil, - nil, - nil, + map[string][]string{ + ".": {}, + }, + bufconfig.NewLintConfig( + bufconfig.NewCheckConfig( + bufconfig.FileVersionV2, + nil, + nil, + nil, + nil, + ), + "", + false, + false, + false, + "", + false, + ), + bufconfig.NewBreakingConfig( + bufconfig.NewCheckConfig( + bufconfig.FileVersionV2, + nil, + nil, + nil, + nil, + ), + false, + ), ) if err != nil { return err From 2c841f711a7033ccd7d2bd8aec0d6362207fb771 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Wed, 13 Dec 2023 18:36:33 -0500 Subject: [PATCH 16/29] only remove workspace modules from the new buf.lock --- private/buf/bufmigrate/migrator.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 7d16a2fc7f..1faa859fbc 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -501,6 +501,9 @@ func (m *migrator) buildBufYAMLAndBufLock( // We are only removing lock entries that are in the workspace. If a lock // entry is not in the workspace, it could be an indirect // A lock entry could be for an indirect dependenceny not listed in deps in buf.yaml. + if _, ok := m.moduleNameToParentFile[moduleFullName]; !ok { + continue + } depModuleToKeys[moduleFullName] = append(depModuleToKeys[moduleFullName], depModuleKey) } areDependenciesResolved := true From 51c351790aae83164300eed3bcdc02222e2ca8dd Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Wed, 13 Dec 2023 18:39:55 -0500 Subject: [PATCH 17/29] move migrate out of beta --- private/buf/cmd/buf/buf.go | 4 ++-- private/buf/cmd/buf/command/{beta => }/migrate/migrate.go | 0 private/buf/cmd/buf/command/{beta => }/migrate/usage.gen.go | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename private/buf/cmd/buf/command/{beta => }/migrate/migrate.go (100%) rename private/buf/cmd/buf/command/{beta => }/migrate/usage.gen.go (100%) diff --git a/private/buf/cmd/buf/buf.go b/private/buf/cmd/buf/buf.go index 385c12643b..4f9563a839 100644 --- a/private/buf/cmd/buf/buf.go +++ b/private/buf/cmd/buf/buf.go @@ -33,7 +33,6 @@ import ( "github.com/bufbuild/buf/private/buf/cmd/buf/command/alpha/registry/token/tokenget" "github.com/bufbuild/buf/private/buf/cmd/buf/command/alpha/registry/token/tokenlist" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/graph" - "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/migrate" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/price" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/registry/commit/commitget" "github.com/bufbuild/buf/private/buf/cmd/buf/command/beta/registry/commit/commitlist" @@ -66,6 +65,7 @@ import ( "github.com/bufbuild/buf/private/buf/cmd/buf/command/generate" "github.com/bufbuild/buf/private/buf/cmd/buf/command/lint" "github.com/bufbuild/buf/private/buf/cmd/buf/command/lsfiles" + "github.com/bufbuild/buf/private/buf/cmd/buf/command/migrate" "github.com/bufbuild/buf/private/buf/cmd/buf/command/mod/modclearcache" "github.com/bufbuild/buf/private/buf/cmd/buf/command/mod/modinit" "github.com/bufbuild/buf/private/buf/cmd/buf/command/mod/modlsbreakingrules" @@ -111,6 +111,7 @@ func NewRootCommand(name string) *appcmd.Command { breaking.NewCommand("breaking", builder), generate.NewCommand("generate", builder), lsfiles.NewCommand("ls-files", builder), + migrate.NewCommand("migrate", builder), // TODO: still need to port push.NewCommand("push", builder), convert.NewCommand("convert", builder), @@ -141,7 +142,6 @@ func NewRootCommand(name string) *appcmd.Command { Short: "Beta commands. Unstable and likely to change", SubCommands: []*appcmd.Command{ graph.NewCommand("graph", builder), - migrate.NewCommand("migrate", builder), price.NewCommand("price", builder), stats.NewCommand("stats", builder), studioagent.NewCommand("studio-agent", builder), diff --git a/private/buf/cmd/buf/command/beta/migrate/migrate.go b/private/buf/cmd/buf/command/migrate/migrate.go similarity index 100% rename from private/buf/cmd/buf/command/beta/migrate/migrate.go rename to private/buf/cmd/buf/command/migrate/migrate.go diff --git a/private/buf/cmd/buf/command/beta/migrate/usage.gen.go b/private/buf/cmd/buf/command/migrate/usage.gen.go similarity index 100% rename from private/buf/cmd/buf/command/beta/migrate/usage.gen.go rename to private/buf/cmd/buf/command/migrate/usage.gen.go From dfcb3f49b9010876b4c40fe2006d5b4c657d5ec0 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 14 Dec 2023 11:06:18 -0500 Subject: [PATCH 18/29] update command description --- private/buf/cmd/buf/command/migrate/migrate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private/buf/cmd/buf/command/migrate/migrate.go b/private/buf/cmd/buf/command/migrate/migrate.go index 9dbdc24285..695415be20 100644 --- a/private/buf/cmd/buf/command/migrate/migrate.go +++ b/private/buf/cmd/buf/command/migrate/migrate.go @@ -43,7 +43,7 @@ func NewCommand( return &appcmd.Command{ Use: name, Short: `Migrate configuration to the latest version`, - Long: `Migrate specified configuration files to the latest version.`, + Long: `Migrate configuration files at the specified directories or paths to the latest version.`, Args: appcmd.MaximumNArgs(0), Run: builder.NewRunFunc( func(ctx context.Context, container appext.Container) error { From e81555b212e6b18c8c7992d9d8d3ed9101240667 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 14 Dec 2023 12:13:59 -0500 Subject: [PATCH 19/29] if workspace dir is the only one specified, destination dir is workspace dir --- private/buf/bufmigrate/bufmigrate.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index ece635f129..913b4e6b25 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -49,11 +49,15 @@ func Migrate( if err != nil { return err } + destionationDirectory := "." + if migrateOptions.workspaceDirectory != "" && len(migrateOptions.moduleDirectories) == 0 { + destionationDirectory = migrateOptions.workspaceDirectory + } migrator := newMigrator( logger, clientProvider, bucket, - ".", + destionationDirectory, ) if migrateOptions.workspaceDirectory != "" { // TODO: should we allow multiple workspaces? From 7d7101dcb4a8a809d1f58b26c85c7b6b027bf041 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 14 Dec 2023 13:15:04 -0500 Subject: [PATCH 20/29] remove some TODOs --- private/buf/bufmigrate/migrator.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 1faa859fbc..54d32ab4a1 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -201,8 +201,6 @@ func (m *migrator) addModuleDirectory( if _, ok := m.filesToDelete[bufYAMLPath]; ok { return nil } - // TODO: transform paths so that they are relative to the new buf.yaml v2 or module root (depending on buf.yaml v2 semantics) - // Paths include RootToExcludes, IgnorePaths, IgnoreIDOrCategoryToPaths switch bufYAML.FileVersion() { case bufconfig.FileVersionV1Beta1: if len(bufYAML.ModuleConfigs()) != 1 { @@ -238,9 +236,7 @@ func (m *migrator) addModuleDirectory( bufconfig.FileVersionV2, lintRuleNames, lintConfig.ExceptIDsAndCategories(), - // TODO: filter these paths by root lintConfig.IgnorePaths(), - // TODO: filter these paths by root lintConfig.IgnoreIDOrCategoryToPaths(), ), lintConfig.EnumZeroValueSuffix(), @@ -261,9 +257,7 @@ func (m *migrator) addModuleDirectory( bufconfig.FileVersionV2, breakingRuleNames, breakingConfig.ExceptIDsAndCategories(), - // TODO: filter these paths by root breakingConfig.IgnorePaths(), - // TODO: filter these paths by root breakingConfig.IgnoreIDOrCategoryToPaths(), ), breakingConfig.IgnoreUnstablePackages(), @@ -307,7 +301,6 @@ func (m *migrator) addModuleDirectory( bufconfig.FileVersionV2, lintConfig.UseIDsAndCategories(), lintConfig.ExceptIDsAndCategories(), - // TODO: paths lintConfig.IgnorePaths(), lintConfig.IgnoreIDOrCategoryToPaths(), ), @@ -324,7 +317,6 @@ func (m *migrator) addModuleDirectory( bufconfig.FileVersionV2, breakingConfig.UseIDsAndCategories(), breakingConfig.ExceptIDsAndCategories(), - // TODO: paths breakingConfig.IgnorePaths(), breakingConfig.IgnoreIDOrCategoryToPaths(), ), @@ -564,6 +556,7 @@ func (m *migrator) buildBufYAMLAndBufLock( } return bufYAML, bufLock, nil } + // TODO: remove entire if-clause when commit service is implemented if true { resolvedDepModuleRefs := make([]bufmodule.ModuleRef, 0, len(depModuleToRefs)) for _, depModuleRefs := range depModuleToRefs { From 7769f5555c4516759736b05499341ecf8934e5b3 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 14 Dec 2023 13:23:45 -0500 Subject: [PATCH 21/29] transform exclude paths --- private/bufpkg/bufconfig/buf_yaml_file.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/private/bufpkg/bufconfig/buf_yaml_file.go b/private/bufpkg/bufconfig/buf_yaml_file.go index 12987b04b5..717bbec1f1 100644 --- a/private/bufpkg/bufconfig/buf_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_yaml_file.go @@ -559,6 +559,9 @@ func writeBufYAMLFile(writer io.Writer, bufYAMLFile BufYAMLFile) error { ) for _, moduleConfig := range bufYAMLFile.ModuleConfigs() { moduleDirPath := moduleConfig.DirPath() + joinDirPath := func(importPath string) string { + return filepath.Join(moduleDirPath, importPath) + } externalModule := externalBufYAMLFileModuleV2{ Directory: moduleDirPath, } @@ -573,14 +576,11 @@ func writeBufYAMLFile(writer io.Writer, bufYAMLFile BufYAMLFile) error { if !ok { return syserror.Newf("had rootToExcludes without key \".\" for NewModuleConfig with FileVersion %v", fileVersion) } - externalModule.Excludes = excludes + externalModule.Excludes = slicesext.Map(excludes, joinDirPath) // All already sorted. lintConfig := moduleConfig.LintConfig() externalModule.Lint.Use = lintConfig.UseIDsAndCategories() externalModule.Lint.Except = lintConfig.ExceptIDsAndCategories() - joinDirPath := func(importPath string) string { - return filepath.Join(moduleDirPath, importPath) - } externalModule.Lint.Ignore = slicesext.Map(lintConfig.IgnorePaths(), joinDirPath) externalModule.Lint.IgnoreOnly = make(map[string][]string, len(lintConfig.IgnoreIDOrCategoryToPaths())) for idOrCategory, importPaths := range lintConfig.IgnoreIDOrCategoryToPaths() { From 8dd13fa2204290e09c937c606349d5c832d78073 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 14 Dec 2023 13:38:47 -0500 Subject: [PATCH 22/29] fix --- private/buf/bufmigrate/migrator.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 54d32ab4a1..8ee1b8fa2c 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -482,7 +482,7 @@ func (m *migrator) buildBufYAMLAndBufLock( depModuleToRefs := make(map[string][]bufmodule.ModuleRef) for _, depModuleRef := range m.moduleDependencies { moduleFullName := depModuleRef.ModuleFullName().String() - if _, ok := m.moduleNameToParentFile[moduleFullName]; !ok { + if _, ok := m.moduleNameToParentFile[moduleFullName]; ok { continue } depModuleToRefs[moduleFullName] = append(depModuleToRefs[moduleFullName], depModuleRef) @@ -490,10 +490,9 @@ func (m *migrator) buildBufYAMLAndBufLock( depModuleToKeys := make(map[string][]bufmodule.ModuleKey) for _, depModuleKey := range m.depModuleKeys { moduleFullName := depModuleKey.ModuleFullName().String() - // We are only removing lock entries that are in the workspace. If a lock - // entry is not in the workspace, it could be an indirect - // A lock entry could be for an indirect dependenceny not listed in deps in buf.yaml. - if _, ok := m.moduleNameToParentFile[moduleFullName]; !ok { + // We are only removing lock entries that are in the workspace. A lock entry + // could be for an indirect dependenceny not listed in deps in any buf.yaml. + if _, ok := m.moduleNameToParentFile[moduleFullName]; ok { continue } depModuleToKeys[moduleFullName] = append(depModuleToKeys[moduleFullName], depModuleKey) From d0d07f1c5d90a3fa0dc8b4d3d0ecbf2eed875545 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 14 Dec 2023 16:09:10 -0500 Subject: [PATCH 23/29] produce minimal diff in lint and breaking rules --- private/buf/bufmigrate/migrator.go | 181 +++++++++++++++++++---------- 1 file changed, 119 insertions(+), 62 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 8ee1b8fa2c..5e745d6aa4 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -222,46 +222,14 @@ func (m *migrator) addModuleDirectory( sortedRoots := slicesext.MapKeysToSortedSlice(moduleConfig.RootToExcludes()) for _, root := range sortedRoots { excludes := moduleConfig.RootToExcludes()[root] - lintConfig := moduleConfig.LintConfig() - // TODO: this list expands to individual rules, we could process - // this list and make it shorter by substituting some rules with - // a single group, if all rules in that group are present. - lintRules, err := buflint.RulesForConfig(lintConfig) + lintConfigForRoot, err := equivalentLintConfigInV2(moduleConfig.LintConfig()) if err != nil { return err } - lintRuleNames := slicesext.Map(lintRules, func(rule bufcheck.Rule) string { return rule.ID() }) - lintConfigForRoot := bufconfig.NewLintConfig( - bufconfig.NewCheckConfig( - bufconfig.FileVersionV2, - lintRuleNames, - lintConfig.ExceptIDsAndCategories(), - lintConfig.IgnorePaths(), - lintConfig.IgnoreIDOrCategoryToPaths(), - ), - lintConfig.EnumZeroValueSuffix(), - lintConfig.RPCAllowSameRequestResponse(), - lintConfig.RPCAllowGoogleProtobufEmptyRequests(), - lintConfig.RPCAllowGoogleProtobufEmptyResponses(), - lintConfig.ServiceSuffix(), - lintConfig.AllowCommentIgnores(), - ) - breakingConfig := moduleConfig.BreakingConfig() - breakingRules, err := bufbreaking.RulesForConfig(breakingConfig) + breakingConfigForRoot, err := equivalentBreakingConfigInV2(moduleConfig.BreakingConfig()) if err != nil { return err } - breakingRuleNames := slicesext.Map(breakingRules, func(rule bufcheck.Rule) string { return rule.ID() }) - breakingConfigForRoot := bufconfig.NewBreakingConfig( - bufconfig.NewCheckConfig( - bufconfig.FileVersionV2, - breakingRuleNames, - breakingConfig.ExceptIDsAndCategories(), - breakingConfig.IgnorePaths(), - breakingConfig.IgnoreIDOrCategoryToPaths(), - ), - breakingConfig.IgnoreUnstablePackages(), - ) dirPathRelativeToDestination, err := filepath.Rel( m.destinationDir, filepath.Join( @@ -294,34 +262,14 @@ func (m *migrator) addModuleDirectory( return syserror.Newf("expect exactly 1 module config from buf yaml, got %d", len(bufYAML.ModuleConfigs())) } moduleConfig := bufYAML.ModuleConfigs()[0] - // use the same lint and breaking config, except that they are v2. - lintConfig := moduleConfig.LintConfig() - lintConfig = bufconfig.NewLintConfig( - bufconfig.NewCheckConfig( - bufconfig.FileVersionV2, - lintConfig.UseIDsAndCategories(), - lintConfig.ExceptIDsAndCategories(), - lintConfig.IgnorePaths(), - lintConfig.IgnoreIDOrCategoryToPaths(), - ), - lintConfig.EnumZeroValueSuffix(), - lintConfig.RPCAllowSameRequestResponse(), - lintConfig.RPCAllowGoogleProtobufEmptyRequests(), - lintConfig.RPCAllowGoogleProtobufEmptyResponses(), - lintConfig.ServiceSuffix(), - lintConfig.AllowCommentIgnores(), - ) - breakingConfig := moduleConfig.BreakingConfig() - breakingConfig = bufconfig.NewBreakingConfig( - bufconfig.NewCheckConfig( - bufconfig.FileVersionV2, - breakingConfig.UseIDsAndCategories(), - breakingConfig.ExceptIDsAndCategories(), - breakingConfig.IgnorePaths(), - breakingConfig.IgnoreIDOrCategoryToPaths(), - ), - breakingConfig.IgnoreUnstablePackages(), - ) + lintConfig, err := equivalentLintConfigInV2(moduleConfig.LintConfig()) + if err != nil { + return err + } + breakingConfig, err := equivalentBreakingConfigInV2(moduleConfig.BreakingConfig()) + if err != nil { + return err + } dirPathRelativeToDestination, err := filepath.Rel(m.destinationDir, filepath.Dir(bufYAMLPath)) if err != nil { return err @@ -771,3 +719,112 @@ func getCommitIDToCommit( } return commitIDToCommit, nil } + +func equivalentLintConfigInV2(lintConfig bufconfig.LintConfig) (bufconfig.LintConfig, error) { + equivalentCheckConfigV2, err := equivalentCheckConfigInV2( + lintConfig, + func(checkConfig bufconfig.CheckConfig) ([]bufcheck.Rule, error) { + lintConfig := bufconfig.NewLintConfig( + checkConfig, + lintConfig.EnumZeroValueSuffix(), + lintConfig.RPCAllowSameRequestResponse(), + lintConfig.RPCAllowGoogleProtobufEmptyRequests(), + lintConfig.RPCAllowGoogleProtobufEmptyResponses(), + lintConfig.ServiceSuffix(), + lintConfig.AllowCommentIgnores(), + ) + return buflint.RulesForConfig(lintConfig) + }, + ) + if err != nil { + return nil, err + } + return bufconfig.NewLintConfig( + equivalentCheckConfigV2, + lintConfig.EnumZeroValueSuffix(), + lintConfig.RPCAllowSameRequestResponse(), + lintConfig.RPCAllowGoogleProtobufEmptyRequests(), + lintConfig.RPCAllowGoogleProtobufEmptyResponses(), + lintConfig.ServiceSuffix(), + lintConfig.AllowCommentIgnores(), + ), nil +} + +func equivalentBreakingConfigInV2(breakingConfig bufconfig.BreakingConfig) (bufconfig.BreakingConfig, error) { + equivalentCheckConfigV2, err := equivalentCheckConfigInV2( + breakingConfig, + func(checkConfig bufconfig.CheckConfig) ([]bufcheck.Rule, error) { + breakingConfig := bufconfig.NewBreakingConfig( + checkConfig, + breakingConfig.IgnoreUnstablePackages(), + ) + return bufbreaking.RulesForConfig(breakingConfig) + }, + ) + if err != nil { + return nil, err + } + return bufconfig.NewBreakingConfig( + equivalentCheckConfigV2, + breakingConfig.IgnoreUnstablePackages(), + ), nil +} + +func equivalentCheckConfigInV2( + checkConfig bufconfig.CheckConfig, + getRulesFunc func(bufconfig.CheckConfig) ([]bufcheck.Rule, error), +) (bufconfig.CheckConfig, error) { + expectedRules, err := getRulesFunc(checkConfig) + if err != nil { + return nil, err + } + expectedIDs := slicesext.Map( + expectedRules, + func(rule bufcheck.Rule) string { + return rule.ID() + }, + ) + simplyTranslatedCheckConfig := bufconfig.NewCheckConfig( + bufconfig.FileVersionV2, + checkConfig.UseIDsAndCategories(), + checkConfig.ExceptIDsAndCategories(), + checkConfig.IgnorePaths(), + checkConfig.IgnoreIDOrCategoryToPaths(), + ) + simplyTranslatedRules, err := getRulesFunc(simplyTranslatedCheckConfig) + if err != nil { + return nil, err + } + simplyTranslatedIDs := slicesext.Map( + simplyTranslatedRules, + func(rule bufcheck.Rule) string { + return rule.ID() + }, + ) + if slicesext.ElementsEqual(expectedIDs, simplyTranslatedIDs) { + return simplyTranslatedCheckConfig, nil + } + expectedIDsMap := slicesext.ToStructMap(expectedIDs) + simplyTranslatedIDsMap := slicesext.ToStructMap(simplyTranslatedIDs) + extraUse := slicesext.Filter( + expectedIDs, + func(expectedID string) bool { + _, ok := simplyTranslatedIDsMap[expectedID] + return !ok + }, + ) + extraExcept := slicesext.Filter( + simplyTranslatedIDs, + func(simplyTranslatedID string) bool { + _, ok := expectedIDsMap[simplyTranslatedID] + return !ok + }, + ) + return bufconfig.NewCheckConfig( + bufconfig.FileVersionV2, + append(checkConfig.UseIDsAndCategories(), extraUse...), + append(checkConfig.ExceptIDsAndCategories(), extraExcept...), + checkConfig.IgnorePaths(), + checkConfig.IgnoreIDOrCategoryToPaths(), + ), nil +} From 76ed0acbd36a8e8feb09c9f8748c2a80d4cd1832 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 14 Dec 2023 16:10:46 -0500 Subject: [PATCH 24/29] remove TODOs on roots to excludes because they are already resolved --- private/buf/bufmigrate/migrator.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 5e745d6aa4..99bfe1c0e9 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -243,7 +243,6 @@ func (m *migrator) addModuleDirectory( configForRoot, err := bufconfig.NewModuleConfig( dirPathRelativeToDestination, moduleFullName, - // TODO: make them relative to what they should be relative to. map[string][]string{".": excludes}, lintConfigForRoot, breakingConfigForRoot, @@ -257,7 +256,6 @@ func (m *migrator) addModuleDirectory( } m.moduleDependencies = append(m.moduleDependencies, bufYAML.ConfiguredDepModuleRefs()...) case bufconfig.FileVersionV1: - // TODO: smiliar to the above, make paths (root to excludes, lint ignore, ...) relative to the correct root (either buf.yaml v2 or module root) if len(bufYAML.ModuleConfigs()) != 1 { return syserror.Newf("expect exactly 1 module config from buf yaml, got %d", len(bufYAML.ModuleConfigs())) } @@ -277,7 +275,6 @@ func (m *migrator) addModuleDirectory( moduleConfig, err = bufconfig.NewModuleConfig( dirPathRelativeToDestination, moduleConfig.ModuleFullName(), - // TODO: paths moduleConfig.RootToExcludes(), lintConfig, breakingConfig, From 290bbb8daf5069ece76ed88f83905d40df399934 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 21 Dec 2023 00:38:05 -0500 Subject: [PATCH 25/29] post merge --- private/buf/bufmigrate/migrator.go | 6 ++++++ private/buf/cmd/buf/command/migrate/migrate.go | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 99bfe1c0e9..b89d454e54 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -35,6 +35,7 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufcheck/buflint" "github.com/bufbuild/buf/private/bufpkg/bufconfig" "github.com/bufbuild/buf/private/bufpkg/bufmodule" + "github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleapi" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/syserror" @@ -297,6 +298,11 @@ func (m *migrator) addModuleDirectory( ctx, m.rootBucket, moduleDir, + bufconfig.BufLockFileWithDigestResolver( + func(ctx context.Context, remote, commitID string) (bufcas.Digest, error) { + return bufmoduleapi.CommitIDToDigest(ctx, m.clientProvider, remote, commitID) + }, + ), ) if errors.Is(errors.Unwrap(err), fs.ErrNotExist) { return nil diff --git a/private/buf/cmd/buf/command/migrate/migrate.go b/private/buf/cmd/buf/command/migrate/migrate.go index 695415be20..78b8fbada9 100644 --- a/private/buf/cmd/buf/command/migrate/migrate.go +++ b/private/buf/cmd/buf/command/migrate/migrate.go @@ -131,7 +131,6 @@ func run( return bufmigrate.Migrate( ctx, container.Logger(), - // TODO: Do we want to add a flag --disable-symlinks? storageos.NewProvider(storageos.ProviderWithSymlinks()), bufapi.NewClientProvider(clientConfig), migrateOptions..., From 942d5d8153910494b6f037e39fc77edbd59e156d Mon Sep 17 00:00:00 2001 From: bufdev Date: Thu, 21 Dec 2023 12:55:43 -0500 Subject: [PATCH 26/29] commit --- private/buf/bufmigrate/bufmigrate.go | 93 ++++++++++++----------- private/buf/bufmigrate/bufmigrate_test.go | 22 ------ private/buf/bufmigrate/migrator.go | 66 +++++++++------- 3 files changed, 88 insertions(+), 93 deletions(-) delete mode 100644 private/buf/bufmigrate/bufmigrate_test.go diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index 913b4e6b25..66fa384ce0 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -17,22 +17,27 @@ package bufmigrate import ( "context" "errors" - "fmt" "io" - "path/filepath" "github.com/bufbuild/buf/private/bufpkg/bufapi" - "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage/storageos" - "go.uber.org/zap" ) -// Migrate migrate buf configuration files. +// Migrate migrates buf configuration files. +// +// TODO: document behavior with different examples of module and workspace directories, ie what happens +// when I specify a module directory that is within a workspace directory I always specify, what happens +// if I don't specify the module directory, what if I only specify some module directories in a +// workspace directory, etc. func Migrate( ctx context.Context, - logger *zap.Logger, + messageWriter io.Writer, storageProvider storageos.Provider, clientProvider bufapi.ClientProvider, + // TODO: use these values + workspaceDirPaths []string, + moduleDirPaths []string, + generateTemplatePaths []string, options ...MigrateOption, ) (retErr error) { migrateOptions := newMigrateOptions() @@ -54,14 +59,14 @@ func Migrate( destionationDirectory = migrateOptions.workspaceDirectory } migrator := newMigrator( - logger, + messageWriter, clientProvider, bucket, destionationDirectory, ) if migrateOptions.workspaceDirectory != "" { // TODO: should we allow multiple workspaces? - if err := migrator.addWorkspace( + if err := migrator.addWorkspaceDirectory( ctx, migrateOptions.workspaceDirectory, ); err != nil { @@ -84,7 +89,7 @@ func Migrate( } } if migrateOptions.dryRun { - return migrator.migrateAsDryRun(ctx, migrateOptions.dryRunWriter) + return migrator.migrateAsDryRun(ctx) } return migrator.migrate(ctx) } @@ -94,52 +99,52 @@ type MigrateOption func(*migrateOptions) // MigrateAsDryRun write to the writer the summary of the changes to be made, // without writing to the disk. -func MigrateAsDryRun(writer io.Writer) MigrateOption { +func MigrateAsDryRun() MigrateOption { return func(migrateOptions *migrateOptions) { migrateOptions.dryRun = true - migrateOptions.dryRunWriter = writer } } -// MigrateWorkspaceDirectory migrates buf.work.yaml, and all buf.yamls in directories -// pointed to by this workspace, as well as their co-resident buf.locks. -func MigrateWorkspaceDirectory(directory string) (MigrateOption, error) { - // TODO: Looking at IsLocal's doc, it seems to validate for what we want: relative and does not jump context. - if !filepath.IsLocal(directory) { - return nil, fmt.Errorf("%s is not a relative path", directory) - } - return func(migrateOptions *migrateOptions) { - migrateOptions.workspaceDirectory = filepath.Clean(directory) - }, nil -} +// TODO: move this validaton inside Migrate -// MigrateModuleDirectories migrates buf.yamls buf.locks in directories. -func MigrateModuleDirectories(directories []string) (MigrateOption, error) { - for _, path := range directories { - if !filepath.IsLocal(path) { - return nil, fmt.Errorf("%s is not a relative path", path) - } - } - return func(migrateOptions *migrateOptions) { - migrateOptions.moduleDirectories = slicesext.Map(directories, filepath.Clean) - }, nil -} +//// MigrateWorkspaceDirectory migrates buf.work.yaml, and all buf.yamls in directories +//// pointed to by this workspace, as well as their co-resident buf.locks. +//func MigrateWorkspaceDirectory(directory string) (MigrateOption, error) { +//// TODO: Looking at IsLocal's doc, it seems to validate for what we want: relative and does not jump context. +//if !filepath.IsLocal(directory) { +//return nil, fmt.Errorf("%s is not a relative path", directory) +//} +//return func(migrateOptions *migrateOptions) { +//migrateOptions.workspaceDirectory = filepath.Clean(directory) +//}, nil +//} -// MigrateGenerationTemplates migrates buf.gen.yamls. -func MigrateGenerationTemplates(paths []string) MigrateOption { - return func(migrateOptions *migrateOptions) { - migrateOptions.bufGenYAMLPaths = slicesext.Map(paths, filepath.Clean) - } -} +//// MigrateModuleDirectories migrates buf.yamls buf.locks in directories. +//func MigrateModuleDirectories(directories []string) (MigrateOption, error) { +//for _, path := range directories { +//if !filepath.IsLocal(path) { +//return nil, fmt.Errorf("%s is not a relative path", path) +//} +//} +//return func(migrateOptions *migrateOptions) { +//migrateOptions.moduleDirectories = slicesext.Map(directories, filepath.Clean) +//}, nil +//} + +//// MigrateGenerationTemplates migrates buf.gen.yamls. +//func MigrateGenerationTemplates(paths []string) MigrateOption { +//return func(migrateOptions *migrateOptions) { +//migrateOptions.bufGenYAMLPaths = slicesext.Map(paths, filepath.Clean) +//} +//} /// *** PRIVATE *** type migrateOptions struct { - dryRun bool - dryRunWriter io.Writer - workspaceDirectory string - moduleDirectories []string - bufGenYAMLPaths []string + dryRun bool + //workspaceDirectory string + //moduleDirectories []string + //bufGenYAMLPaths []string } func newMigrateOptions() *migrateOptions { diff --git a/private/buf/bufmigrate/bufmigrate_test.go b/private/buf/bufmigrate/bufmigrate_test.go deleted file mode 100644 index 49cb721c0f..0000000000 --- a/private/buf/bufmigrate/bufmigrate_test.go +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2020-2023 Buf Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package bufmigrate - -import "testing" - -func TestMigrate(t *testing.T) { - // TODO - t.Parallel() -} diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index b89d454e54..01203d811a 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -40,11 +40,10 @@ import ( "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/syserror" "go.uber.org/multierr" - "go.uber.org/zap" ) type migrator struct { - logger *zap.Logger + messageWriter io.Writer clientProvider bufapi.ClientProvider // the bucket at "." rootBucket storage.ReadWriteBucket @@ -62,13 +61,14 @@ type migrator struct { } func newMigrator( - logger *zap.Logger, + // usually stderr + messageWriter io.Writer, clientProvider bufapi.ClientProvider, rootBucket storage.ReadWriteBucket, destinationDir string, ) *migrator { return &migrator{ - logger: logger, + messageWriter: messageWriter, clientProvider: clientProvider, destinationDir: destinationDir, rootBucket: rootBucket, @@ -93,11 +93,12 @@ func (m *migrator) addBufGenYAML( return err } if bufGenYAML.FileVersion() == bufconfig.FileVersionV2 { - m.logger.Sugar().Warnf("%s is already in v2", bufGenYAMLPath) + m.warnf("%s is a v2 file, no migration required", bufGenYAMLPath) return nil } if typeConfig := bufGenYAML.GenerateConfig().GenerateTypeConfig(); typeConfig != nil && len(typeConfig.IncludeTypes()) > 0 { - m.logger.Sugar().Warnf( + // TODO: what does this sentence mean? Get someone else to read it and understand it without any explanation. + m.warnf( "%s has types configuration and is migrated to v2 without it. To add these types your v2 configuration, first add an input and add these types to it.", ) } @@ -111,7 +112,8 @@ func (m *migrator) addBufGenYAML( return nil } -func (m *migrator) addWorkspace( +// TODO: document is workspaceDirectory always relative to root of bucket? +func (m *migrator) addWorkspaceDirectory( ctx context.Context, workspaceDirectory string, ) (retErr error) { @@ -121,7 +123,7 @@ func (m *migrator) addWorkspace( workspaceDirectory, ) if errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("%q does not have a workspace configuration file", workspaceDirectory) + return fmt.Errorf("%q does not have a workspace configuration file (i.e. typically a buf.work.yaml)", workspaceDirectory) } if err != nil { return err @@ -135,7 +137,8 @@ func (m *migrator) addWorkspace( return nil } -// both buf.yaml and buf.lock +// both buf.yaml and buf.lock TODO what does this mean? +// TODO: document is workspaceDirectory always relative to root of bucket? func (m *migrator) addModuleDirectory( ctx context.Context, // moduleDir is the relative path (relative to ".") to the module directory @@ -212,7 +215,7 @@ func (m *migrator) addModuleDirectory( // module configs, but they cannot share the same module full name. moduleFullName := moduleConfig.ModuleFullName() if len(moduleConfig.RootToExcludes()) > 1 && moduleFullName != nil { - m.logger.Sugar().Warnf( + m.warnf( "%s has name %s and multiple roots. These roots are now separate unnamed modules.", bufYAMLPath, moduleFullName.String(), @@ -288,7 +291,7 @@ func (m *migrator) addModuleDirectory( } m.moduleDependencies = append(m.moduleDependencies, bufYAML.ConfiguredDepModuleRefs()...) case bufconfig.FileVersionV2: - m.logger.Sugar().Warnf("%s is already at v2", bufYAMLPath) + m.warnf("%s is already at v2", bufYAMLPath) return nil default: return syserror.Newf("unexpected version: %v", bufYAML.FileVersion()) @@ -327,13 +330,9 @@ func (m *migrator) addModuleDirectory( return nil } -func (m *migrator) migrateAsDryRun( - ctx context.Context, - writer io.Writer, -) (retErr error) { - fmt.Fprintf( - writer, - "In an actual run, these files will be removed:\n%s\n\nThe following files will be overwritten or created:\n\n", +func (m *migrator) migrateAsDryRun(ctx context.Context) (retErr error) { + m.infof( + "In an actual run, these files will be removed:\n%s\n\nThe following files will be overwritten or created:\n", strings.Join(slicesext.MapKeysToSortedSlice(m.filesToDelete), "\n"), ) if len(m.moduleConfigs) > 0 { @@ -345,9 +344,8 @@ func (m *migrator) migrateAsDryRun( if err := bufconfig.WriteBufYAMLFile(&bufYAMLBuffer, migratedBufYAML); err != nil { return err } - fmt.Fprintf( - writer, - "%s:\n%s\n", + m.infof( + "%s:\n%s", filepath.Join(m.destinationDir, bufconfig.DefaultBufYAMLFileName), bufYAMLBuffer.String(), ) @@ -356,9 +354,8 @@ func (m *migrator) migrateAsDryRun( if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { return err } - fmt.Fprintf( - writer, - "%s:\n%s\n", + m.infof( + "%s:\n%s", filepath.Join(m.destinationDir, bufconfig.DefaultBufLockFileName), bufLockBuffer.String(), ) @@ -369,9 +366,8 @@ func (m *migrator) migrateAsDryRun( if err := bufconfig.WriteBufGenYAMLFile(&bufGenYAMLBuffer, migratedBufGenYAML); err != nil { return err } - fmt.Fprintf( - writer, - "%s will be written:\n%s\n", + m.infof( + "%s will be written:\n%s", bufGenYAMLPath, bufGenYAMLBuffer.String(), ) @@ -587,6 +583,22 @@ func (m *migrator) appendModuleConfig(moduleConfig bufconfig.ModuleConfig, paren return nil } +func (m *migrator) info(message string) { + _, _ = m.messageWriter.Write([]byte(fmt.Sprintf("%s\n", message))) +} + +func (m *migrator) infof(format string, args ...any) { + _, _ = m.messageWriter.Write([]byte(fmt.Sprintf("%s\n", fmt.Sprintf(format, args...)))) +} + +func (m *migrator) warn(message string) { + _, _ = m.messageWriter.Write([]byte(fmt.Sprintf("Warning: %s\n", message))) +} + +func (m *migrator) warnf(format string, args ...any) { + _, _ = m.messageWriter.Write([]byte(fmt.Sprintf("Warning: %s\n", fmt.Sprintf(format, args...)))) +} + func resolvedDeclaredAndLockedDependencies( moduleToRefToCommit map[string]map[string]*modulev1beta1.Commit, commitIDToCommit map[string]*modulev1beta1.Commit, From 73a82d5c98058c83c131c438637bf490d5057513 Mon Sep 17 00:00:00 2001 From: bufdev Date: Thu, 21 Dec 2023 12:57:53 -0500 Subject: [PATCH 27/29] commit --- private/buf/bufmigrate/bufmigrate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index 66fa384ce0..467c6dd272 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -25,7 +25,7 @@ import ( // Migrate migrates buf configuration files. // -// TODO: document behavior with different examples of module and workspace directories, ie what happens +// TODO: document behavior with different examples of module and workspace directories, ie what happens // when I specify a module directory that is within a workspace directory I always specify, what happens // if I don't specify the module directory, what if I only specify some module directories in a // workspace directory, etc. From 62610fc6c09490eaa0ab8ffac745823b188d24c2 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Thu, 21 Dec 2023 20:09:28 -0500 Subject: [PATCH 28/29] updates --- private/buf/bufmigrate/bufmigrate.go | 118 ++++---- private/buf/bufmigrate/migrator.go | 257 +++++++++++------- .../buf/cmd/buf/command/migrate/migrate.go | 63 ++--- private/bufpkg/bufconfig/buf_yaml_file.go | 1 + 4 files changed, 246 insertions(+), 193 deletions(-) diff --git a/private/buf/bufmigrate/bufmigrate.go b/private/buf/bufmigrate/bufmigrate.go index 467c6dd272..a8606cb58f 100644 --- a/private/buf/bufmigrate/bufmigrate.go +++ b/private/buf/bufmigrate/bufmigrate.go @@ -17,24 +17,46 @@ package bufmigrate import ( "context" "errors" + "fmt" "io" + "path/filepath" "github.com/bufbuild/buf/private/bufpkg/bufapi" + "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage/storageos" ) // Migrate migrates buf configuration files. // -// TODO: document behavior with different examples of module and workspace directories, ie what happens -// when I specify a module directory that is within a workspace directory I always specify, what happens -// if I don't specify the module directory, what if I only specify some module directories in a -// workspace directory, etc. +// A buf.yaml v2 is written if any workspace directory or module directory is +// specified. The modules directories in the buf.yaml v2 will contain: +// +// - directories at moduleDirPaths +// - directories pointed to by buf.work.yamls at workspaceDirPaths +// +// More specifically: +// +// - If a workspace is specified, then all of its module directories are also migrated, +// regardless whether these module directories are specified in moduleDirPaths. Same +// behavior with multiple workspaces. For example, if workspace foo has directories +// bar and baz, then specifying foo, foo + bar and foo + bar + baz are the same. +// - If a workspace is specfied, and modules not from this workspace are specified, the +// buf.yaml will contain all directories from the workspace, as well as the module +// directories specified. +// - If only module directories are specified, then the buf.yaml will contain exactly +// these directories. +// - If a module specified is within some workspace not from workspaceDirPaths, we migrate +// the module directory only (updating/deciding on this behavior is still a TODO). +// - If only one workspace directory is specified and no module directory is specified, +// the buf.yaml will be written at /buf.yaml. Otherwise, it will +// be written at ./buf.yaml. +// +// Each generation template will be overwritten by a file in v2. func Migrate( ctx context.Context, messageWriter io.Writer, storageProvider storageos.Provider, clientProvider bufapi.ClientProvider, - // TODO: use these values workspaceDirPaths []string, moduleDirPaths []string, generateTemplatePaths []string, @@ -44,9 +66,38 @@ func Migrate( for _, option := range options { option(migrateOptions) } - if migrateOptions.workspaceDirectory == "" && len(migrateOptions.moduleDirectories) == 0 && len(migrateOptions.bufGenYAMLPaths) == 0 { + if len(workspaceDirPaths) == 0 && len(moduleDirPaths) == 0 && len(generateTemplatePaths) == 0 { return errors.New("no directory or file specified") } + var err error + // Directories cannot jump context because in the migrated buf.yaml v2, each + // directory path cannot jump context. I.e. it's not valid to have `- directory: ..` + // in a buf.yaml v2. + workspaceDirPaths, err = slicesext.MapError( + workspaceDirPaths, + func(workspaceDirPath string) (string, error) { + if !filepath.IsLocal(workspaceDirPath) { + return "", fmt.Errorf("%s is not a relative path", workspaceDirPath) + } + return filepath.Clean(workspaceDirPath), nil + }, + ) + if err != nil { + return err + } + moduleDirPaths, err = slicesext.MapError( + moduleDirPaths, + func(moduleDirPath string) (string, error) { + if !filepath.IsLocal(moduleDirPath) { + return "", fmt.Errorf("%s is not a relative path", moduleDirPath) + } + return filepath.Clean(moduleDirPath), nil + }, + ) + if err != nil { + return err + } + generateTemplatePaths = slicesext.Map(generateTemplatePaths, filepath.Clean) bucket, err := storageProvider.NewReadWriteBucket( ".", storageos.ReadWriteBucketWithSymlinksIfSupported(), @@ -55,8 +106,8 @@ func Migrate( return err } destionationDirectory := "." - if migrateOptions.workspaceDirectory != "" && len(migrateOptions.moduleDirectories) == 0 { - destionationDirectory = migrateOptions.workspaceDirectory + if len(workspaceDirPaths) == 1 && len(moduleDirPaths) == 0 { + destionationDirectory = workspaceDirPaths[0] } migrator := newMigrator( messageWriter, @@ -64,18 +115,18 @@ func Migrate( bucket, destionationDirectory, ) - if migrateOptions.workspaceDirectory != "" { - // TODO: should we allow multiple workspaces? + for _, workspaceDirPath := range workspaceDirPaths { if err := migrator.addWorkspaceDirectory( ctx, - migrateOptions.workspaceDirectory, + workspaceDirPath, ); err != nil { return err } } - for _, bufYAMLPath := range migrateOptions.moduleDirectories { - // TODO: read upwards to make sure it's not in a workspace + for _, bufYAMLPath := range moduleDirPaths { + // TODO: read upwards to make sure it's not in a workspace. // i.e. for ./foo/bar/buf.yaml, check none of "./foo", ".", "../", "../..", and etc. is a workspace. + // The logic for this is in getMapPathAndSubDirPath from buffetch/internal if err := migrator.addModuleDirectory( ctx, bufYAMLPath, @@ -83,7 +134,7 @@ func Migrate( return err } } - for _, bufGenYAMLPath := range migrateOptions.bufGenYAMLPaths { + for _, bufGenYAMLPath := range generateTemplatePaths { if err := migrator.addBufGenYAML(bufGenYAMLPath); err != nil { return err } @@ -97,54 +148,17 @@ func Migrate( // MigrateOption is a migrate option. type MigrateOption func(*migrateOptions) -// MigrateAsDryRun write to the writer the summary of the changes to be made, -// without writing to the disk. +// MigrateAsDryRun print the summary of the changes to be made, without writing to the disk. func MigrateAsDryRun() MigrateOption { return func(migrateOptions *migrateOptions) { migrateOptions.dryRun = true } } -// TODO: move this validaton inside Migrate - -//// MigrateWorkspaceDirectory migrates buf.work.yaml, and all buf.yamls in directories -//// pointed to by this workspace, as well as their co-resident buf.locks. -//func MigrateWorkspaceDirectory(directory string) (MigrateOption, error) { -//// TODO: Looking at IsLocal's doc, it seems to validate for what we want: relative and does not jump context. -//if !filepath.IsLocal(directory) { -//return nil, fmt.Errorf("%s is not a relative path", directory) -//} -//return func(migrateOptions *migrateOptions) { -//migrateOptions.workspaceDirectory = filepath.Clean(directory) -//}, nil -//} - -//// MigrateModuleDirectories migrates buf.yamls buf.locks in directories. -//func MigrateModuleDirectories(directories []string) (MigrateOption, error) { -//for _, path := range directories { -//if !filepath.IsLocal(path) { -//return nil, fmt.Errorf("%s is not a relative path", path) -//} -//} -//return func(migrateOptions *migrateOptions) { -//migrateOptions.moduleDirectories = slicesext.Map(directories, filepath.Clean) -//}, nil -//} - -//// MigrateGenerationTemplates migrates buf.gen.yamls. -//func MigrateGenerationTemplates(paths []string) MigrateOption { -//return func(migrateOptions *migrateOptions) { -//migrateOptions.bufGenYAMLPaths = slicesext.Map(paths, filepath.Clean) -//} -//} - /// *** PRIVATE *** type migrateOptions struct { dryRun bool - //workspaceDirectory string - //moduleDirectories []string - //bufGenYAMLPaths []string } func newMigrateOptions() *migrateOptions { diff --git a/private/buf/bufmigrate/migrator.go b/private/buf/bufmigrate/migrator.go index 01203d811a..ed92ce2b3a 100644 --- a/private/buf/bufmigrate/migrator.go +++ b/private/buf/bufmigrate/migrator.go @@ -15,7 +15,6 @@ package bufmigrate import ( - "bytes" "context" "errors" "fmt" @@ -38,6 +37,7 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleapi" "github.com/bufbuild/buf/private/pkg/slicesext" "github.com/bufbuild/buf/private/pkg/storage" + "github.com/bufbuild/buf/private/pkg/stringutil" "github.com/bufbuild/buf/private/pkg/syserror" "go.uber.org/multierr" ) @@ -78,6 +78,13 @@ func newMigrator( } } +// addBufGenYAML adds a buf.gen.yaml to the list of files to migrate. It returns nil +// nil if the file is already in v2. +// +// If the file is in v1 and has a 'types' section on the top level, this function will +// ignore 'types' and print a warning, while migrating everything else in the file. +// +// bufGenYAMLPath is relative to the call site of CLI or an absolute path. func (m *migrator) addBufGenYAML( bufGenYAMLPath string, ) (retErr error) { @@ -99,12 +106,19 @@ func (m *migrator) addBufGenYAML( if typeConfig := bufGenYAML.GenerateConfig().GenerateTypeConfig(); typeConfig != nil && len(typeConfig.IncludeTypes()) > 0 { // TODO: what does this sentence mean? Get someone else to read it and understand it without any explanation. m.warnf( - "%s has types configuration and is migrated to v2 without it. To add these types your v2 configuration, first add an input and add these types to it.", + "%s is a v1 generation template with a top-level 'types' section including %s. In a v2 generation template, 'types' can"+ + " only exist within an input in the 'inputs' section. Since the migration command does not have information"+ + " on inputs, the migrated generation will not have an 'inputs' section. To add these types in the migrated file, you can"+ + " first add an input to 'inputs' and then add these types to the input.", + bufGenYAMLPath, + stringutil.SliceToHumanString(typeConfig.IncludeTypes()), ) } + // No special transformation needed, writeBufGenYAMLFile handles it correctly. migratedBufGenYAML := bufconfig.NewBufGenYAMLFile( bufconfig.FileVersionV2, bufGenYAML.GenerateConfig(), + // Types is always nil in v2. nil, ) m.filesToDelete[bufGenYAMLPath] = struct{}{} @@ -112,7 +126,11 @@ func (m *migrator) addBufGenYAML( return nil } -// TODO: document is workspaceDirectory always relative to root of bucket? +// addWorkspaceDirectory adds the buf.work.yaml at the root of the workspace directory +// to the list of files to migrate, the buf.yamls and buf.locks at the root of each +// directory pointed to by this workspace. +// +// workspaceDirectory is relative to the root bucket of the migrator. func (m *migrator) addWorkspaceDirectory( ctx context.Context, workspaceDirectory string, @@ -137,25 +155,30 @@ func (m *migrator) addWorkspaceDirectory( return nil } -// both buf.yaml and buf.lock TODO what does this mean? -// TODO: document is workspaceDirectory always relative to root of bucket? +// addModuleDirectory adds buf.yaml and buf.lock at the root of moduleDir to the list +// of files to migrate. More specifically, it adds module configs and dependency module +// keys to the migrator. +// +// moduleDir is relative to the root bucket of the migrator. func (m *migrator) addModuleDirectory( ctx context.Context, - // moduleDir is the relative path (relative to ".") to the module directory moduleDir string, ) (retErr error) { + // First get module configs from the buf.yaml at moduleDir. bufYAML, err := bufconfig.GetBufYAMLFileForPrefix( ctx, m.rootBucket, moduleDir, ) if errors.Is(errors.Unwrap(err), fs.ErrNotExist) { - moduleDirInMigratedBufYAML, err := filepath.Rel(m.destinationDir, moduleDir) + // If buf.yaml isn't present, migration does not fail. Instead we add an + // empty module config representing this directory. + moduleRootRelativeToDestination, err := filepath.Rel(m.destinationDir, moduleDir) if err != nil { return err } - moduleConfig, err := bufconfig.NewModuleConfig( - moduleDirInMigratedBufYAML, + emptyModuleConfig, err := bufconfig.NewModuleConfig( + moduleRootRelativeToDestination, nil, map[string][]string{ ".": {}, @@ -190,30 +213,37 @@ func (m *migrator) addModuleDirectory( return err } if err := m.appendModuleConfig( - moduleConfig, + emptyModuleConfig, filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName), ); err != nil { return err } - // Assume there is no co-resident buf.lock + // Assuming there is no co-resident buf.lock when there is no buf.yaml, + // we return early here. return nil } if err != nil { return err } - bufYAMLPath := filepath.Join(moduleDir, bufconfig.DefaultBufYAMLFileName) + bufYAMLPath := filepath.Join(moduleDir, bufYAML.FileName()) + // If this module is already visited, we don't add it for a second time. It's + // possbile to visit the same module directory twice when the user specifies both + // a workspace and a module in this workspace. if _, ok := m.filesToDelete[bufYAMLPath]; ok { return nil } switch bufYAML.FileVersion() { case bufconfig.FileVersionV1Beta1: if len(bufYAML.ModuleConfigs()) != 1 { + // This should never happen because it's guaranteed by the bufYAMLFile interface. return syserror.Newf("expect exactly 1 module config from buf yaml, got %d", len(bufYAML.ModuleConfigs())) } moduleConfig := bufYAML.ModuleConfigs()[0] - // If a v1beta buf.yaml has multiple roots, they are split into multiple - // module configs, but they cannot share the same module full name. moduleFullName := moduleConfig.ModuleFullName() + // If a buf.yaml v1beta1 has a non-empty name and multiple roots, the + // resulting buf.yaml v2 should have these roots as module directories, + // but they should not share the same module name. Instead we just give + // them empty module names. if len(moduleConfig.RootToExcludes()) > 1 && moduleFullName != nil { m.warnf( "%s has name %s and multiple roots. These roots are now separate unnamed modules.", @@ -222,63 +252,65 @@ func (m *migrator) addModuleDirectory( ) moduleFullName = nil } - // Iterate through root-to-excludes in deterministic order. + // Each root in buf.yaml v1beta1 should become its own module config in v2, + // and we iterate through these roots in deterministic order. sortedRoots := slicesext.MapKeysToSortedSlice(moduleConfig.RootToExcludes()) for _, root := range sortedRoots { - excludes := moduleConfig.RootToExcludes()[root] - lintConfigForRoot, err := equivalentLintConfigInV2(moduleConfig.LintConfig()) + moduleRootRelativeToDestination, err := filepath.Rel( + m.destinationDir, + filepath.Join(moduleDir, root), + ) if err != nil { return err } - breakingConfigForRoot, err := equivalentBreakingConfigInV2(moduleConfig.BreakingConfig()) + lintConfigForRoot, err := equivalentLintConfigInV2(moduleConfig.LintConfig()) if err != nil { return err } - dirPathRelativeToDestination, err := filepath.Rel( - m.destinationDir, - filepath.Join( - filepath.Dir(bufYAMLPath), - root, - ), - ) + breakingConfigForRoot, err := equivalentBreakingConfigInV2(moduleConfig.BreakingConfig()) if err != nil { return err } - configForRoot, err := bufconfig.NewModuleConfig( - dirPathRelativeToDestination, + moduleConfigForRoot, err := bufconfig.NewModuleConfig( + moduleRootRelativeToDestination, moduleFullName, - map[string][]string{".": excludes}, + // We do not need to handle paths in root-to-excludes, lint or breaking config specially, + // because the paths are transformed correctly by readBufYAMLFile and writeBufYAMLFile. + map[string][]string{".": moduleConfig.RootToExcludes()[root]}, lintConfigForRoot, breakingConfigForRoot, ) if err != nil { return err } - if err := m.appendModuleConfig(configForRoot, bufYAMLPath); err != nil { + if err := m.appendModuleConfig(moduleConfigForRoot, bufYAMLPath); err != nil { return err } } m.moduleDependencies = append(m.moduleDependencies, bufYAML.ConfiguredDepModuleRefs()...) case bufconfig.FileVersionV1: if len(bufYAML.ModuleConfigs()) != 1 { + // This should never happen because it's guaranteed by the bufYAMLFile interface. return syserror.Newf("expect exactly 1 module config from buf yaml, got %d", len(bufYAML.ModuleConfigs())) } moduleConfig := bufYAML.ModuleConfigs()[0] - lintConfig, err := equivalentLintConfigInV2(moduleConfig.LintConfig()) + moduleRootRelativeToDestination, err := filepath.Rel(m.destinationDir, filepath.Dir(bufYAMLPath)) if err != nil { return err } - breakingConfig, err := equivalentBreakingConfigInV2(moduleConfig.BreakingConfig()) + lintConfig, err := equivalentLintConfigInV2(moduleConfig.LintConfig()) if err != nil { return err } - dirPathRelativeToDestination, err := filepath.Rel(m.destinationDir, filepath.Dir(bufYAMLPath)) + breakingConfig, err := equivalentBreakingConfigInV2(moduleConfig.BreakingConfig()) if err != nil { return err } moduleConfig, err = bufconfig.NewModuleConfig( - dirPathRelativeToDestination, + moduleRootRelativeToDestination, moduleConfig.ModuleFullName(), + // We do not need to handle paths in root-to-excludes, lint or breaking config specially, + // because the paths are transformed correctly by readBufYAMLFile and writeBufYAMLFile. moduleConfig.RootToExcludes(), lintConfig, breakingConfig, @@ -291,12 +323,15 @@ func (m *migrator) addModuleDirectory( } m.moduleDependencies = append(m.moduleDependencies, bufYAML.ConfiguredDepModuleRefs()...) case bufconfig.FileVersionV2: - m.warnf("%s is already at v2", bufYAMLPath) + m.warnf("%s is a v2 file, no migration required", bufYAMLPath) return nil default: return syserror.Newf("unexpected version: %v", bufYAML.FileVersion()) } m.filesToDelete[bufYAMLPath] = struct{}{} + // Now we read buf.lock and add its lock entries to the list of candidate lock entries + // for the migrated buf.lock. These lock entries are candiates because different buf.locks + // can have lock entries for the same module but for different commits. bufLockFile, err := bufconfig.GetBufLockFileForPrefix( ctx, m.rootBucket, @@ -313,64 +348,65 @@ func (m *migrator) addModuleDirectory( if err != nil { return err } - bufLockFilePath := filepath.Join(moduleDir, bufconfig.DefaultBufLockFileName) + bufLockFilePath := filepath.Join(moduleDir, bufLockFile.FileName()) // We don't need to check whether it's already in the map, but because if it were, // its co-resident buf.yaml would also have been a duplicate and made this // function return at an earlier point. m.filesToDelete[bufLockFilePath] = struct{}{} + m.hasSeenBufLock = true switch bufLockFile.FileVersion() { case bufconfig.FileVersionV1Beta1, bufconfig.FileVersionV1: m.depModuleKeys = append(m.depModuleKeys, bufLockFile.DepModuleKeys()...) case bufconfig.FileVersionV2: - return fmt.Errorf("%s is already at v2", bufLockFilePath) + m.warnf("%s is a v2 file, no migration required", bufLockFilePath) + return nil default: return syserror.Newf("unrecognized version: %v", bufLockFile.FileVersion()) } - m.hasSeenBufLock = true return nil } func (m *migrator) migrateAsDryRun(ctx context.Context) (retErr error) { - m.infof( - "In an actual run, these files will be removed:\n%s\n\nThe following files will be overwritten or created:\n", - strings.Join(slicesext.MapKeysToSortedSlice(m.filesToDelete), "\n"), - ) + if len(m.filesToDelete) > 0 { + m.infof( + "In an actual run, these files will be removed:\n%s\n\nThe following files will be overwritten or created:\n", + strings.Join(slicesext.MapKeysToSortedSlice(m.filesToDelete), "\n"), + ) + } else { + m.info("In an actual run:\n") + } + // We create a buf.yaml if we have seen visited any module directory. Note + // we add a module config even for a module directory without a buf.yaml. if len(m.moduleConfigs) > 0 { migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) if err != nil { return err } - var bufYAMLBuffer bytes.Buffer - if err := bufconfig.WriteBufYAMLFile(&bufYAMLBuffer, migratedBufYAML); err != nil { - return err - } m.infof( - "%s:\n%s", - filepath.Join(m.destinationDir, bufconfig.DefaultBufYAMLFileName), - bufYAMLBuffer.String(), + "%s will be written:\n", + filepath.Join(m.destinationDir, bufconfig.DefaultBufWorkYAMLFileName), ) + if err := bufconfig.WriteBufYAMLFile(m.messageWriter, migratedBufYAML); err != nil { + return err + } if migratedBufLock != nil { - var bufLockBuffer bytes.Buffer - if err := bufconfig.WriteBufLockFile(&bufLockBuffer, migratedBufLock); err != nil { - return err - } m.infof( - "%s:\n%s", + "%s will be written:\n", filepath.Join(m.destinationDir, bufconfig.DefaultBufLockFileName), - bufLockBuffer.String(), ) + if err := bufconfig.WriteBufLockFile(m.messageWriter, migratedBufLock); err != nil { + return err + } } } for bufGenYAMLPath, migratedBufGenYAML := range m.pathToMigratedBufGenYAML { - var bufGenYAMLBuffer bytes.Buffer - if err := bufconfig.WriteBufGenYAMLFile(&bufGenYAMLBuffer, migratedBufGenYAML); err != nil { - return err - } m.infof( - "%s will be written:\n%s", + "%s will be written:\n", bufGenYAMLPath, - bufGenYAMLBuffer.String(), ) + if err := bufconfig.WriteBufGenYAMLFile(m.messageWriter, migratedBufGenYAML); err != nil { + return err + } } return nil } @@ -379,7 +415,8 @@ func (m *migrator) migrate( ctx context.Context, ) (retErr error) { for bufGenYAMLPath, migratedBufGenYAML := range m.pathToMigratedBufGenYAML { - file, err := os.OpenFile(bufGenYAMLPath, os.O_WRONLY|os.O_TRUNC, 0644) + // os.Create truncates the existing file. + file, err := os.Create(bufGenYAMLPath) if err != nil { return err } @@ -390,6 +427,8 @@ func (m *migrator) migrate( return err } } + // We create a buf.yaml if we have seen visited any module directory. Note + // we add a module config even for a module directory without a buf.yaml. if len(m.moduleConfigs) > 0 { migratedBufYAML, migratedBufLock, err := m.buildBufYAMLAndBufLock(ctx) if err != nil { @@ -422,90 +461,99 @@ func (m *migrator) migrate( return nil } -// If no error, the BufYAMLFile returned is never nil, but the BufLockFile returne may be nil. +// If this function doesn't return an error, the BufYAMLFile returned is never nil, +// but the BufLockFile returned may be nil. func (m *migrator) buildBufYAMLAndBufLock( ctx context.Context, ) (bufconfig.BufYAMLFile, bufconfig.BufLockFile, error) { - depModuleToRefs := make(map[string][]bufmodule.ModuleRef) - for _, depModuleRef := range m.moduleDependencies { - moduleFullName := depModuleRef.ModuleFullName().String() + // module full name --> the list of declared dependencies that are this module. + depModuleToDeclaredRefs := make(map[string][]bufmodule.ModuleRef) + for _, declaredRef := range m.moduleDependencies { + moduleFullName := declaredRef.ModuleFullName().String() + // If a declared dependency also shows up in the workspace, it's not a dependency. if _, ok := m.moduleNameToParentFile[moduleFullName]; ok { continue } - depModuleToRefs[moduleFullName] = append(depModuleToRefs[moduleFullName], depModuleRef) + depModuleToDeclaredRefs[moduleFullName] = append(depModuleToDeclaredRefs[moduleFullName], declaredRef) } - depModuleToKeys := make(map[string][]bufmodule.ModuleKey) - for _, depModuleKey := range m.depModuleKeys { - moduleFullName := depModuleKey.ModuleFullName().String() + // module full name --> the list of lock entries that are this module. + depModuleToLockEntries := make(map[string][]bufmodule.ModuleKey) + for _, lockEntry := range m.depModuleKeys { + moduleFullName := lockEntry.ModuleFullName().String() + // If a declared dependency also shows up in the workspace, it's not a dependency. + // // We are only removing lock entries that are in the workspace. A lock entry // could be for an indirect dependenceny not listed in deps in any buf.yaml. if _, ok := m.moduleNameToParentFile[moduleFullName]; ok { continue } - depModuleToKeys[moduleFullName] = append(depModuleToKeys[moduleFullName], depModuleKey) + depModuleToLockEntries[moduleFullName] = append(depModuleToLockEntries[moduleFullName], lockEntry) } + // This will be set to false if the duplicate dependencies cannot be resolved locally. areDependenciesResolved := true - for depModule, depModuleRefs := range depModuleToRefs { + for depModule, declaredRefs := range depModuleToDeclaredRefs { refStringToRef := make(map[string]bufmodule.ModuleRef) - for _, ref := range depModuleRefs { + for _, ref := range declaredRefs { // Add ref even if ref.Ref() is empty. Therefore, slicesext.ToValuesMap is not used. refStringToRef[ref.Ref()] = ref } - // If there are both buf.build/foo/bar and buf.build/foo/bar:some_ref, take the latter. + // If there are both buf.build/foo/bar and buf.build/foo/bar:some_ref, the former will + // not be used. if len(refStringToRef) > 1 { delete(refStringToRef, "") } + depModuleToDeclaredRefs[depModule] = slicesext.MapValuesToSlice(refStringToRef) if len(refStringToRef) > 1 { areDependenciesResolved = false } - depModuleToRefs[depModule] = slicesext.MapValuesToSlice(refStringToRef) } - for depModule, depModuleKeys := range depModuleToKeys { + for depModule, lockEntries := range depModuleToLockEntries { commitIDToKey := slicesext.ToValuesMap( - depModuleKeys, + lockEntries, func(moduleKey bufmodule.ModuleKey) string { return moduleKey.CommitID() }, ) + depModuleToLockEntries[depModule] = slicesext.MapValuesToSlice(commitIDToKey) if len(commitIDToKey) > 1 { areDependenciesResolved = false } - depModuleToKeys[depModule] = slicesext.MapValuesToSlice(commitIDToKey) } if areDependenciesResolved { - resolvedDepModuleRefs := make([]bufmodule.ModuleRef, 0, len(depModuleToRefs)) - for _, depModuleRefs := range depModuleToRefs { - // depModuleRefs is guaranteed to have length 1 - resolvedDepModuleRefs = append(resolvedDepModuleRefs, depModuleRefs...) + resolvedDeclaredRefs := make([]bufmodule.ModuleRef, 0, len(depModuleToDeclaredRefs)) + for _, depModuleRefs := range depModuleToDeclaredRefs { + // depModuleRefs is guaranteed to have length 1, because areDependenciesResolved is true. + resolvedDeclaredRefs = append(resolvedDeclaredRefs, depModuleRefs...) } bufYAML, err := bufconfig.NewBufYAMLFile( bufconfig.FileVersionV2, m.moduleConfigs, - resolvedDepModuleRefs, + resolvedDeclaredRefs, ) if err != nil { return nil, nil, err } - resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleToKeys)) - for _, depModuleKeys := range depModuleToKeys { - resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys...) + resolvedLockEntries := make([]bufmodule.ModuleKey, 0, len(depModuleToLockEntries)) + for _, lockEntry := range depModuleToLockEntries { + resolvedLockEntries = append(resolvedLockEntries, lockEntry...) } var bufLock bufconfig.BufLockFile if m.hasSeenBufLock { bufLock, err = bufconfig.NewBufLockFile( bufconfig.FileVersionV2, - resolvedDepModuleKeys, + resolvedLockEntries, ) if err != nil { return nil, nil, err } } + // bufLock could be nil here, but that's OK, see docs for this function. return bufYAML, bufLock, nil } // TODO: remove entire if-clause when commit service is implemented if true { - resolvedDepModuleRefs := make([]bufmodule.ModuleRef, 0, len(depModuleToRefs)) - for _, depModuleRefs := range depModuleToRefs { + resolvedDepModuleRefs := make([]bufmodule.ModuleRef, 0, len(depModuleToDeclaredRefs)) + for _, depModuleRefs := range depModuleToDeclaredRefs { resolvedDepModuleRefs = append(resolvedDepModuleRefs, depModuleRefs[0]) } bufYAML, err := bufconfig.NewBufYAMLFile( @@ -516,8 +564,8 @@ func (m *migrator) buildBufYAMLAndBufLock( if err != nil { return nil, nil, err } - resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleToKeys)) - for _, depModuleKeys := range depModuleToKeys { + resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(depModuleToLockEntries)) + for _, depModuleKeys := range depModuleToLockEntries { resolvedDepModuleKeys = append(resolvedDepModuleKeys, depModuleKeys[0]) } var bufLock bufconfig.BufLockFile @@ -544,8 +592,8 @@ func (m *migrator) buildBufYAMLAndBufLock( resolvedDepModuleRefs, resolvedDepModuleKeys, err := resolvedDeclaredAndLockedDependencies( moduleToRefToCommit, commitIDToCommit, - depModuleToRefs, - depModuleToKeys, + depModuleToDeclaredRefs, + depModuleToLockEntries, ) if err != nil { return nil, nil, err @@ -618,8 +666,10 @@ func resolvedDeclaredAndLockedDependencies( } resolvedDepModuleKeys := make([]bufmodule.ModuleKey, 0, len(moduleFullNameToLockKeys)) for moduleFullName, lockKeys := range moduleFullNameToLockKeys { - resolvedRef := depModuleFullNameToResolvedRef[moduleFullName] - if resolvedRef.Ref() != "" { + resolvedRef, ok := depModuleFullNameToResolvedRef[moduleFullName] + if ok && resolvedRef.Ref() != "" { + // If we have already picked a pinned dependency ref for this dependency, + // we use that as the lock entry as well. resolvedCommit := moduleToRefToCommit[moduleFullName][resolvedRef.Ref()] key, err := bufmodule.NewModuleKey( resolvedRef.ModuleFullName(), @@ -632,7 +682,7 @@ func resolvedDeclaredAndLockedDependencies( resolvedDepModuleKeys = append(resolvedDepModuleKeys, key) continue } - // Use the lastest + // Otherwise, we pick the latest key from the buf.locks we have read. sort.Slice(lockKeys, func(i, j int) bool { iTime := commitIDToCommit[lockKeys[i].CommitID()].GetCreateTime().AsTime() jTime := commitIDToCommit[lockKeys[j].CommitID()].GetCreateTime().AsTime() @@ -641,6 +691,7 @@ func resolvedDeclaredAndLockedDependencies( resolvedDepModuleKeys = append(resolvedDepModuleKeys, lockKeys[0]) } resolvedDeclaredDependencies := slicesext.MapValuesToSlice(depModuleFullNameToResolvedRef) + // Sort the resolved dependencies for deterministic results. sort.Slice(resolvedDeclaredDependencies, func(i, j int) bool { return resolvedDeclaredDependencies[i].ModuleFullName().String() < resolvedDeclaredDependencies[j].ModuleFullName().String() }) @@ -785,10 +836,14 @@ func equivalentBreakingConfigInV2(breakingConfig bufconfig.BreakingConfig) (bufc ), nil } +// Returns an equivalent check config with (close to) minimal difference in the +// list of rules and categories specified. func equivalentCheckConfigInV2( checkConfig bufconfig.CheckConfig, getRulesFunc func(bufconfig.CheckConfig) ([]bufcheck.Rule, error), ) (bufconfig.CheckConfig, error) { + // These are the rules we want the returned config to have in effect. + // i.e. getRulesFunc(returnedConfig) should return this list. expectedRules, err := getRulesFunc(checkConfig) if err != nil { return nil, err @@ -799,6 +854,8 @@ func equivalentCheckConfigInV2( return rule.ID() }, ) + // First create a check config with the exact same UseIDsAndCategories. This + // is a simple translation. It may or may not be equivalent to the given check config. simplyTranslatedCheckConfig := bufconfig.NewCheckConfig( bufconfig.FileVersionV2, checkConfig.UseIDsAndCategories(), @@ -817,18 +874,20 @@ func equivalentCheckConfigInV2( }, ) if slicesext.ElementsEqual(expectedIDs, simplyTranslatedIDs) { + // If the simple translation is equivalent to before, use it. return simplyTranslatedCheckConfig, nil } + // Otherwise, find what's missing and what's extra. expectedIDsMap := slicesext.ToStructMap(expectedIDs) simplyTranslatedIDsMap := slicesext.ToStructMap(simplyTranslatedIDs) - extraUse := slicesext.Filter( + missingIDs := slicesext.Filter( expectedIDs, func(expectedID string) bool { _, ok := simplyTranslatedIDsMap[expectedID] return !ok }, ) - extraExcept := slicesext.Filter( + extraIDs := slicesext.Filter( simplyTranslatedIDs, func(simplyTranslatedID string) bool { _, ok := expectedIDsMap[simplyTranslatedID] @@ -837,8 +896,8 @@ func equivalentCheckConfigInV2( ) return bufconfig.NewCheckConfig( bufconfig.FileVersionV2, - append(checkConfig.UseIDsAndCategories(), extraUse...), - append(checkConfig.ExceptIDsAndCategories(), extraExcept...), + append(checkConfig.UseIDsAndCategories(), missingIDs...), + append(checkConfig.ExceptIDsAndCategories(), extraIDs...), checkConfig.IgnorePaths(), checkConfig.IgnoreIDOrCategoryToPaths(), ), nil diff --git a/private/buf/cmd/buf/command/migrate/migrate.go b/private/buf/cmd/buf/command/migrate/migrate.go index 78b8fbada9..4ce0ca24c7 100644 --- a/private/buf/cmd/buf/command/migrate/migrate.go +++ b/private/buf/cmd/buf/command/migrate/migrate.go @@ -16,7 +16,6 @@ package migrate import ( "context" - "errors" "github.com/bufbuild/buf/private/buf/bufcli" "github.com/bufbuild/buf/private/buf/bufmigrate" @@ -28,10 +27,10 @@ import ( ) const ( - workspaceDirectoryFlagName = "workspace" - moduleDirectoriesFlagName = "module" - bufGenYAMLPathFlagName = "template" - dryRunFlagName = "dry-run" + workspaceDirectoriesFlagName = "workspace" + moduleDirectoriesFlagName = "module" + bufGenYAMLPathFlagName = "template" + dryRunFlagName = "dry-run" ) // NewCommand returns a new Command. @@ -55,10 +54,10 @@ func NewCommand( } type flags struct { - WorkspaceDirectory string - ModuleDirectories []string - BufGenYAMLPath []string - DryRun bool + WorkspaceDirPaths []string + ModuleDirPaths []string + BufGenYAMLPaths []string + DryRun bool } func newFlags() *flags { @@ -66,17 +65,17 @@ func newFlags() *flags { } func (f *flags) Bind(flagSet *pflag.FlagSet) { - flagSet.StringVar( - &f.WorkspaceDirectory, - workspaceDirectoryFlagName, - "", - "The workspace directory to migrate. Its buf.work.yaml, buf.yamls and buf.locks will be migrated.", + flagSet.StringSliceVar( + &f.WorkspaceDirPaths, + workspaceDirectoriesFlagName, + nil, + "The workspace directories to migrate. buf.work.yaml, buf.yamls and buf.locks will be migrated.", ) flagSet.StringSliceVar( - &f.ModuleDirectories, + &f.ModuleDirPaths, moduleDirectoriesFlagName, nil, - "The buf.yamls to migrate. Its buf.yaml and buf.lock will be migrated", + "The module directories to migrate. buf.yaml and buf.lock will be migrated", ) flagSet.BoolVar( &f.DryRun, @@ -85,7 +84,7 @@ func (f *flags) Bind(flagSet *pflag.FlagSet) { "Print the changes to be made without writing to the disk", ) flagSet.StringSliceVar( - &f.BufGenYAMLPath, + &f.BufGenYAMLPaths, bufGenYAMLPathFlagName, nil, "The paths to the generation templates to migrate", @@ -98,31 +97,8 @@ func run( flags *flags, ) error { var migrateOptions []bufmigrate.MigrateOption - if flags.WorkspaceDirectory != "" { - option, err := bufmigrate.MigrateWorkspaceDirectory(flags.WorkspaceDirectory) - if err != nil { - return err - } - migrateOptions = append(migrateOptions, option) - } - if len(flags.ModuleDirectories) > 0 { - option, err := bufmigrate.MigrateModuleDirectories(flags.ModuleDirectories) - if err != nil { - return err - } - migrateOptions = append(migrateOptions, option) - } - if len(flags.BufGenYAMLPath) > 0 { - migrateOptions = append( - migrateOptions, - bufmigrate.MigrateGenerationTemplates(flags.BufGenYAMLPath), - ) - } - if len(migrateOptions) == 0 { - return errors.New("no directories or files specified") - } if flags.DryRun { - migrateOptions = append(migrateOptions, bufmigrate.MigrateAsDryRun(container.Stdout())) + migrateOptions = append(migrateOptions, bufmigrate.MigrateAsDryRun()) } clientConfig, err := bufcli.NewConnectClientConfig(container) if err != nil { @@ -130,9 +106,12 @@ func run( } return bufmigrate.Migrate( ctx, - container.Logger(), + container.Stderr(), storageos.NewProvider(storageos.ProviderWithSymlinks()), bufapi.NewClientProvider(clientConfig), + flags.WorkspaceDirPaths, + flags.ModuleDirPaths, + flags.BufGenYAMLPaths, migrateOptions..., ) } diff --git a/private/bufpkg/bufconfig/buf_yaml_file.go b/private/bufpkg/bufconfig/buf_yaml_file.go index 717bbec1f1..5acc39f21a 100644 --- a/private/bufpkg/bufconfig/buf_yaml_file.go +++ b/private/bufpkg/bufconfig/buf_yaml_file.go @@ -264,6 +264,7 @@ func newBufYAMLFile( ) return &bufYAMLFile{ fileVersion: fileVersion, + fileName: fileName, moduleConfigs: moduleConfigs, configuredDepModuleRefs: configuredDepModuleRefs, }, nil From 9d9ed2293fce04b8f663b23e09ef7a0a9ece2658 Mon Sep 17 00:00:00 2001 From: Oliver Sun Date: Fri, 22 Dec 2023 12:16:10 -0500 Subject: [PATCH 29/29] fix --- private/bufpkg/bufconfig/buf_lock_file.go | 1 + 1 file changed, 1 insertion(+) diff --git a/private/bufpkg/bufconfig/buf_lock_file.go b/private/bufpkg/bufconfig/buf_lock_file.go index 536cef18a7..29a745498d 100644 --- a/private/bufpkg/bufconfig/buf_lock_file.go +++ b/private/bufpkg/bufconfig/buf_lock_file.go @@ -202,6 +202,7 @@ func newBufLockFile( ) bufLockFile := &bufLockFile{ fileVersion: fileVersion, + fileName: fileName, depModuleKeys: depModuleKeys, } if err := validateV1AndV1Beta1DepsHaveCommits(bufLockFile); err != nil {