From 1bf3f7983503582e8c6d5a10f56f702ded4b043c Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Thu, 18 Jan 2024 12:06:46 +0300 Subject: [PATCH] fix: some small improvements and deprecation logs --- docs/configuration.md | 36 +++++++++++++-------------- internal/config/config.go | 6 +++-- internal/config/load.go | 47 +++++++++++++++++++++++++----------- internal/config/load_test.go | 30 ++++++++++++++--------- internal/config/remote.go | 7 +++--- internal/git/remote.go | 44 +++++++++++++++------------------ internal/lefthook/install.go | 5 ---- internal/lefthook/run.go | 5 ---- testdata/remote.txt | 11 ++++++--- testdata/remotes.txt | 4 ++- 10 files changed, 106 insertions(+), 89 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index a72fac0e..4333f46b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -376,21 +376,21 @@ remote: > :test_tube: This feature is in **Beta** version -You can provide multiple remotes configs if you want to share yours lefthook configurations across many projects. Lefthook will automatically download and merge configurations into your local `lefthook.yml`. +You can provide multiple remote configs if you want to share yours lefthook configurations across many projects. Lefthook will automatically download and merge configurations into your local `lefthook.yml`. -You can use [`extends`](#extends) related to the config file (not absolute paths). +You can use [`extends`](#extends) but the paths must be relative to the remote repository root. -If you provide [`scripts`](#scripts) in a remote file, the [scripts](#source_dir) folder must be in the **root of the repository**. +If you provide [`scripts`](#scripts) in a remote config file, the [scripts](#source_dir) folder must also be in the **root of the repository**. **Note** -Configuration in `remotes` will be merged to configuration in `lefthook.yml`, so the priority will be the following: +The configuration from `remotes` will be merged to the local config using the following priority: -- `lefthook.yml` -- `remotes` -- `lefthook-local.yml` +1. Local main config (`lefthook.yml`) +1. Remote configs (`remotes`) +1. Local overrides (`lefthook-local.yml`) -This can be changed in the future. For convenience, please use `remotes` configuration without any hooks configuration in `lefthook.yml`. +This priority may be changed in the future. For convenience, if you use `remotes`, please don't configure any hooks. ### `git_url` @@ -428,13 +428,13 @@ remotes: ref: v1.0.0 ``` -> **Note** +> :warning: **Note** > -> :warning: If you initially had `ref` option, ran `lefthook install`, and then removed it, lefthook won't decide which branch/tag to use as a ref. So, if you added it once, please, use it always to avoid issues in local setups. +> If you initially had `ref` option, ran `lefthook install`, and then removed it, lefthook won't decide which branch/tag to use as a ref. So, if you added it once, please, use it always to avoid issues in local setups. ### `configs` -**Default:** `- lefthook.yml` +**Default:** `[lefthook.yml]` An optional array of config paths from remote's root. @@ -444,32 +444,32 @@ An optional array of config paths from remote's root. # lefthook.yml remotes: - - git_url: git@github.com:evilmartians/remote + - git_url: git@github.com:evilmartians/lefthook ref: v1.0.0 configs: - examples/ruby-linter.yml - examples/test.yml ``` -More complicated example. +Example with multiple remotes merging multiple configurations. ```yml # lefthook.yml remotes: - - git_url: git@github.com:evilmartians/remote + - git_url: git@github.com:org/lefthook-configs ref: v1.0.0 configs: - examples/ruby-linter.yml - examples/test.yml - - git_url : https://github.com:example/repository + - git_url: https://github.com/org2/lefthook-configs configs: - lefthooks/pre_commit.yml - lefthooks/post_merge.yml - - git_url : https://github.com:example2/repository2 - ref: specific_branch + - git_url: https://github.com/org3/lefthook-configs + ref: feature/new configs: - - example/pre-push.yml + - configs/pre-push.yml ``` diff --git a/internal/config/config.go b/internal/config/config.go index 9c31a3f1..19c72e5f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -24,8 +24,10 @@ type Config struct { NoTTY bool `mapstructure:"no_tty,omitempty"` AssertLefthookInstalled bool `mapstructure:"assert_lefthook_installed,omitempty"` Colors interface{} `mapstructure:"colors,omitempty"` - Remote *Remote `mapstructure:"remote,omitempty"` // Deprecated in favor of Remotes - Remotes []*Remote `mapstructure:"remotes,omitempty"` + + // Deprecated: use Remotes + Remote *Remote `mapstructure:"remote,omitempty"` + Remotes []*Remote `mapstructure:"remotes,omitempty"` Hooks map[string]*Hook `mapstructure:"-"` } diff --git a/internal/config/load.go b/internal/config/load.go index 0c593759..7c1f060d 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -94,8 +94,11 @@ func readOne(fs afero.Fs, path string, names []string) (*viper.Viper, error) { return nil, NotFoundError{fmt.Sprintf("No config files with names %q could not be found in \"%s\"", names, path)} } -// mergeAll merges (.lefthook or lefthook) and (extended config) and (remotes) -// and (.lefthook-local or .lefthook-local) configs. +// mergeAll merges configs using the following order. +// - lefthook/.lefthook +// - files from `extends` +// - files from `remotes` +// - lefthook-local/.lefthook-local. func mergeAll(fs afero.Fs, repo *git.Repository) (*viper.Viper, error) { extends, err := readOne(fs, repo.RootPath, []string{"lefthook", ".lefthook"}) if err != nil { @@ -124,27 +127,24 @@ func mergeAll(fs afero.Fs, repo *git.Repository) (*viper.Viper, error) { return extends, nil } -// mergeRemotes merges remotes config to the current one. +// mergeRemotes merges remote configs to the current one. func mergeRemotes(fs afero.Fs, repo *git.Repository, v *viper.Viper) error { + var remote *Remote // Deprecated var remotes []*Remote - var remote *Remote // Use for backward compatibility err := v.UnmarshalKey("remotes", &remotes) if err != nil { return err } - // Use for backward compatibility + // Deprecated err = v.UnmarshalKey("remote", &remote) if err != nil { return err } - // Use for backward compatibility - // If "remote" key exists, append it to "remotes" + // Backward compatibility if remote != nil { - // Not logged because it's breaking tests - // log.Warn("DEPRECATED: \"remote\" key is deprecated, use \"remotes\" instead") remotes = append(remotes, remote) } @@ -153,10 +153,8 @@ func mergeRemotes(fs afero.Fs, repo *git.Repository, v *viper.Viper) error { continue } - // Use for backward compatibility with "remote(s).config" instead of "remote(s).configs" + // Use for backward compatibility with "remote(s).config" if remote.Config != "" { - // Not logged because it's breaking tests - // log.Warn("DEPRECATED: \"config\" key is deprecated, use \"configs\" instead for remotes") remote.Configs = append(remote.Configs, remote.Config) } @@ -169,7 +167,7 @@ func mergeRemotes(fs afero.Fs, repo *git.Repository, v *viper.Viper) error { configFile := config configPath := filepath.Join(remotePath, configFile) - log.Debugf("Merging remote config: %s", configPath) + log.Debugf("Merging remote config: %s: %s", remote.GitURL, configPath) _, err = fs.Stat(configPath) if err != nil { @@ -267,7 +265,28 @@ func unmarshalConfigs(base, extra *viper.Viper, c *Config) error { return err } - return base.Unmarshal(c) + if err := base.Unmarshal(c); err != nil { + return err + } + + // Deprecation handling + + if c.Remote != nil { + log.Warn("DEPRECATED: \"remote\" option is deprecated and will be omitted in the next major release, use \"remotes\" option instead") + c.Remotes = append(c.Remotes, c.Remote) + } + c.Remote = nil + + for _, remote := range c.Remotes { + if remote.Config != "" { + log.Warn("DEPRECATED: \"remotes\".\"config\" option is deprecated and will be omitted in the next major release, use \"configs\" option instead") + remote.Configs = append(remote.Configs, remote.Config) + } + + remote.Config = "" + } + + return nil } func addHook(hookName string, base, extra *viper.Viper, c *Config) error { diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 4935d361..76f0aa43 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -433,8 +433,10 @@ pre-commit: SourceDir: DefaultSourceDir, SourceDirLocal: DefaultSourceDirLocal, Colors: nil, - Remote: &Remote{ - GitURL: "git@github.com:evilmartians/lefthook", + Remotes: []*Remote{ + { + GitURL: "git@github.com:evilmartians/lefthook", + }, }, Hooks: map[string]*Hook{ "pre-commit": { @@ -488,10 +490,12 @@ pre-commit: SourceDir: DefaultSourceDir, SourceDirLocal: DefaultSourceDirLocal, Colors: nil, - Remote: &Remote{ - GitURL: "git@github.com:evilmartians/lefthook", - Ref: "v1.0.0", - Config: "examples/custom.yml", + Remotes: []*Remote{ + { + GitURL: "git@github.com:evilmartians/lefthook", + Ref: "v1.0.0", + Configs: []string{"examples/custom.yml"}, + }, }, Hooks: map[string]*Hook{ "pre-commit": { @@ -573,9 +577,11 @@ pre-push: SourceDir: DefaultSourceDir, SourceDirLocal: DefaultSourceDirLocal, Colors: nil, - Remote: &Remote{ - GitURL: "https://github.com/evilmartians/lefthook", - Config: "examples/config.yml", + Remotes: []*Remote{ + { + GitURL: "https://github.com/evilmartians/lefthook", + Configs: []string{"examples/config.yml"}, + }, }, Extends: []string{"local-extend.yml"}, Hooks: map[string]*Hook{ @@ -830,9 +836,9 @@ pre-commit: Colors: nil, Remotes: []*Remote{ { - GitURL: "https://github.com/evilmartians/lefthook", - Ref: "v1.0.0", - Config: "examples/custom.yml", + GitURL: "https://github.com/evilmartians/lefthook", + Ref: "v1.0.0", + Configs: []string{"examples/custom.yml"}, }, { GitURL: "https://github.com/evilmartians/lefthook", diff --git a/internal/config/remote.go b/internal/config/remote.go index b45e933a..ebdfca57 100644 --- a/internal/config/remote.go +++ b/internal/config/remote.go @@ -1,9 +1,10 @@ package config type Remote struct { - GitURL string `mapstructure:"git_url" yaml:"git_url" json:"git_url,omitempty" toml:"git_url"` - Ref string `mapstructure:"ref,omitempty" yaml:",omitempty" json:"ref,omitempty" toml:"ref,omitempty"` - Config string `mapstructure:"config,omitempty" yaml:",omitempty" json:"config,omitempty" toml:"config,omitempty"` // Deprecated in favor of Configs + GitURL string `mapstructure:"git_url" yaml:"git_url" json:"git_url,omitempty" toml:"git_url"` + Ref string `mapstructure:"ref,omitempty" yaml:",omitempty" json:"ref,omitempty" toml:"ref,omitempty"` + // Deprecated + Config string `mapstructure:"config,omitempty" yaml:",omitempty" json:"config,omitempty" toml:"config,omitempty"` Configs []string `mapstructure:"configs,omitempty" yaml:",omitempty" json:"configs,omitempty" toml:"configs,omitempty"` } diff --git a/internal/git/remote.go b/internal/git/remote.go index 475140bf..ae72f734 100644 --- a/internal/git/remote.go +++ b/internal/git/remote.go @@ -17,17 +17,9 @@ const ( // RemoteFolder returns the path to the folder where the remote // repository is located. func (r *Repository) RemoteFolder(url string, ref string) string { - directoryName := filepath.Base( - strings.TrimSuffix(url, filepath.Ext(url)), - ) - - if ref != "" { - directoryName = directoryName + "-" + ref - } - return filepath.Join( r.RemotesFolder(), - directoryName, + remoteDirectoryName(url, ref), ) } @@ -40,25 +32,15 @@ func (r *Repository) RemotesFolder() string { // specified as a remote config repository. If successful, the path to the root // of the repository will be returned. func (r *Repository) SyncRemote(url, ref string) error { - remotesPath := filepath.Join(r.InfoPath, remotesFolder) + remotesPath := r.RemotesFolder() err := r.Fs.MkdirAll(remotesPath, remotesFolderMode) if err != nil && !errors.Is(err, os.ErrExist) { return err } - directoryName := filepath.Base( - strings.TrimSuffix(url, filepath.Ext(url)), - ) - - if ref != "" { - directoryName = directoryName + "-" + ref - } - - remotePath := filepath.Join( - remotesPath, - directoryName, - ) + directoryName := remoteDirectoryName(url, ref) + remotePath := filepath.Join(remotesPath, directoryName) _, err = r.Fs.Stat(remotePath) if err == nil { @@ -96,10 +78,10 @@ func (r *Repository) updateRemote(path, ref string) error { return nil } -func (r *Repository) cloneRemote(cwd, directoryName, url, ref string) error { - log.Debugf("Cloning remote config repository: %v/%v", cwd, directoryName) +func (r *Repository) cloneRemote(dest, directoryName, url, ref string) error { + log.Debugf("Cloning remote config repository: %v/%v", dest, directoryName) - cmdClone := []string{"git", "-C", cwd, "clone", "--quiet", "--depth", "1"} + cmdClone := []string{"git", "-C", dest, "clone", "--quiet", "--depth", "1"} if len(ref) > 0 { cmdClone = append(cmdClone, "--branch", ref) } @@ -112,3 +94,15 @@ func (r *Repository) cloneRemote(cwd, directoryName, url, ref string) error { return nil } + +func remoteDirectoryName(url, ref string) string { + name := filepath.Base( + strings.TrimSuffix(url, filepath.Ext(url)), + ) + + if ref != "" { + name = name + "-" + ref + } + + return name +} diff --git a/internal/lefthook/install.go b/internal/lefthook/install.go index 98dbbeec..1724a97e 100644 --- a/internal/lefthook/install.go +++ b/internal/lefthook/install.go @@ -50,11 +50,6 @@ func (l *Lefthook) Install(force bool) error { return err } - // For backward compatibility with single remote config - if cfg.Remote != nil { - cfg.Remotes = append(cfg.Remotes, cfg.Remote) - } - for _, remote := range cfg.Remotes { if remote.Configured() { if err := l.repo.SyncRemote(remote.GitURL, remote.Ref); err != nil { diff --git a/internal/lefthook/run.go b/internal/lefthook/run.go index be51c640..6b7849e4 100644 --- a/internal/lefthook/run.go +++ b/internal/lefthook/run.go @@ -131,11 +131,6 @@ Run 'lefthook install' manually.`, filepath.Join(l.repo.RootPath, cfg.SourceDirLocal), } - // For backward compatibility with single remote config - if cfg.Remote != nil { - cfg.Remotes = append(cfg.Remotes, cfg.Remote) - } - for _, remote := range cfg.Remotes { if remote.Configured() { // Append only source_dir, because source_dir_local doesn't make sense diff --git a/testdata/remote.txt b/testdata/remote.txt index de39429e..72435af1 100644 --- a/testdata/remote.txt +++ b/testdata/remote.txt @@ -11,11 +11,14 @@ remote: ref: v1.4.0 -- lefthook-dump.yml -- +DEPRECATED: "remote" option is deprecated and will be omitted in the next major release, use "remotes" option instead +DEPRECATED: "remotes"."config" option is deprecated and will be omitted in the next major release, use "configs" option instead pre-commit: scripts: good_job.js: runner: node -remote: - config: examples/with_scripts/lefthook.yml - git_url: https://github.com/evilmartians/lefthook - ref: v1.4.0 +remotes: + - git_url: https://github.com/evilmartians/lefthook + ref: v1.4.0 + configs: + - examples/with_scripts/lefthook.yml diff --git a/testdata/remotes.txt b/testdata/remotes.txt index 0870bee7..7858b6e3 100644 --- a/testdata/remotes.txt +++ b/testdata/remotes.txt @@ -15,6 +15,7 @@ remotes: - examples/remote/ping.yml -- lefthook-dump.yml -- +DEPRECATED: "remotes"."config" option is deprecated and will be omitted in the next major release, use "configs" option instead pre-commit: commands: js-lint: @@ -45,7 +46,8 @@ pre-push: remotes: - git_url: https://github.com/evilmartians/lefthook ref: v1.4.0 - config: examples/with_scripts/lefthook.yml + configs: + - examples/with_scripts/lefthook.yml - git_url: https://github.com/evilmartians/lefthook configs: - examples/verbose/lefthook.yml