Permalink
Browse files

Misc minor fixes and refactoring

`skeema add-environment` no longer does a connectivity test of the supplied
host; it just manipulates the .skeema file now.

`skeema lint` had a bug with how it handled --ignore-schema, now fixed.

Some exit codes have been changed from 2 (generic fatal error) to 78 (bad
config) in cases where a configuration problem is more clearly to blame.

`go test` now ignores global option files (/etc/skeema and
/usr/local/etc/skeema) and user-specific option files ~/.my.cnf and ~/.skeema,
to avoid these files affecting the result of tests.

Hidden directories are now consistently ignored everywhere, in terms of option
file parsing. This is to avoid issues with SCM metadata directories.

`skeema push` now handles "schema=*" more consistently: the alphabetically
first schema will always be used as the template.

Minor internal refactors to a few methods to make them more testable.
  • Loading branch information...
evanelias committed May 25, 2018
1 parent 5d154ce commit a6704081ac4ef19c8822221492a3d1d965e21d0e
Showing with 120 additions and 69 deletions.
  1. +10 −3 cmd_add_env.go
  2. +1 −2 cmd_diff.go
  3. +2 −2 cmd_lint.go
  4. +4 −4 cmd_push.go
  5. +31 −26 configutils.go
  6. +12 −6 dir.go
  7. +23 −17 exit.go
  8. +1 −1 shellout.go
  9. +26 −0 shellout_test.go
  10. +3 −6 skeema.go
  11. +7 −2 sqlfile.go
@@ -6,6 +6,7 @@ import (
log "github.com/sirupsen/logrus"
"github.com/skeema/mybase"
"github.com/skeema/tengo"
)
func init() {
@@ -57,14 +58,20 @@ func AddEnvHandler(cfg *mybase.Config) error {
return NewExitValue(CodeBadConfig, "This command should be run against a --dir whose .skeema file already defines a host for another environment")
}
// Create a tengo.Instance representing the supplied host. We intentionally
// don't actually test connectivity here though, since this command only
// manipulates the option file. We can't use dir.FirstInstance() here since
// that checks connectivity.
var inst *tengo.Instance
if !cfg.OnCLI("host") {
return NewExitValue(CodeBadConfig, "`skeema add-environment` requires --host to be supplied on CLI")
}
inst, err := dir.FirstInstance()
if err != nil {
if instances, err := dir.Instances(); err != nil {
return err
} else if inst == nil {
} else if len(instances) == 0 {
return NewExitValue(CodeBadConfig, "Command line did not specify which instance to connect to")
} else {
inst = instances[0]
}
hostOptionFile.SetOptionValue(environment, "host", inst.Host)
@@ -31,8 +31,7 @@ differences were found, or 2+ if an error occurred.`
// DiffHandler is the handler method for `skeema diff`
func DiffHandler(cfg *mybase.Config) error {
// We just delegate to PushHandler, forcing dry-run to be enabled and always
// using concurrency of 1
// We just delegate to PushHandler, forcing dry-run to be enabled
cfg.CLI.OptionValues["dry-run"] = "1"
return PushHandler(cfg)
}
@@ -52,8 +52,8 @@ func LintHandler(cfg *mybase.Config) error {
if ignoreSchema, err := t.Dir.Config.GetRegexp("ignore-schema"); err != nil {
return err
} else if ignoreSchema != nil && ignoreSchema.MatchString(dir.String()) {
log.Warnf("Skipping schema %s because of ignore-schema='%s'", dir, ignoreSchema)
} else if ignoreSchema != nil && ignoreSchema.MatchString(t.SchemaFromDir.Name) {
log.Warnf("Skipping schema %s because of ignore-schema='%s'", t.SchemaFromDir.Name, ignoreSchema)
continue
}
@@ -71,7 +71,7 @@ func PushHandler(cfg *mybase.Config) error {
err = fmt.Errorf("concurrent-instances cannot be less than 1")
}
if err != nil {
return err
return NewExitValue(CodeBadConfig, err.Error())
}
// The 2nd param of dir.TargetGroups indicates that SQLFile errors are to be
@@ -197,17 +197,17 @@ func pushWorker(sps *sharedPushState) {
mods.AllowUnsafe = t.Dir.Config.GetBool("allow-unsafe") || sps.briefOutput
mods.AlgorithmClause, err = t.Dir.Config.GetEnum("alter-algorithm", "INPLACE", "COPY", "DEFAULT")
if err != nil {
sps.setFatalError(err)
sps.setFatalError(NewExitValue(CodeBadConfig, err.Error()))
return
}
mods.LockClause, err = t.Dir.Config.GetEnum("alter-lock", "NONE", "SHARED", "EXCLUSIVE", "DEFAULT")
if err != nil {
sps.setFatalError(err)
sps.setFatalError(NewExitValue(CodeBadConfig, err.Error()))
return
}
ignoreTable, err := t.Dir.Config.GetRegexp("ignore-table")
if err != nil {
sps.setFatalError(err)
sps.setFatalError(NewExitValue(CodeBadConfig, err.Error()))
return
}
for n, tableDiff := range diff.TableDiffs {
@@ -46,34 +46,39 @@ func AddGlobalOptions(cmd *mybase.Command) {
// options. Generally, subcommand handlers should call AddGlobalConfigFiles at
// the top of the method.
func AddGlobalConfigFiles(cfg *mybase.Config) {
globalFilePaths := []string{"/etc/skeema", "/usr/local/etc/skeema"}
home := filepath.Clean(os.Getenv("HOME"))
if home != "" {
globalFilePaths = append(globalFilePaths, path.Join(home, ".my.cnf"), path.Join(home, ".skeema"))
}
for _, path := range globalFilePaths {
f := mybase.NewFile(path)
if !f.Exists() {
continue
}
if err := f.Read(); err != nil {
log.Warnf("Ignoring global option file %s due to read error: %s", f.Path(), err)
continue
}
if strings.HasSuffix(path, ".my.cnf") {
f.IgnoreUnknownOptions = true
}
if err := f.Parse(cfg); err != nil {
log.Warnf("Ignoring global option file %s due to parse error: %s", f.Path(), err)
continue
}
if strings.HasSuffix(path, ".my.cnf") {
_ = f.UseSection("skeema", "client", "mysql") // safe to ignore error (doesn't matter if section doesn't exist)
} else {
_ = f.UseSection(cfg.Get("environment")) // safe to ignore error (doesn't matter if section doesn't exist)
// Most logic in this method needs to be skipped for tests. Otherwise, if the
// user running the test happens to have a ~/.my.cnf, ~/.skeema, /etc/skeema,
// or so forth, it would get picked up by the test.
if !cfg.IsTest {
globalFilePaths := []string{"/etc/skeema", "/usr/local/etc/skeema"}
home := filepath.Clean(os.Getenv("HOME"))
if home != "" {
globalFilePaths = append(globalFilePaths, path.Join(home, ".my.cnf"), path.Join(home, ".skeema"))
}
for _, path := range globalFilePaths {
f := mybase.NewFile(path)
if !f.Exists() {
continue
}
if err := f.Read(); err != nil {
log.Warnf("Ignoring global option file %s due to read error: %s", f.Path(), err)
continue
}
if strings.HasSuffix(path, ".my.cnf") {
f.IgnoreUnknownOptions = true
}
if err := f.Parse(cfg); err != nil {
log.Warnf("Ignoring global option file %s due to parse error: %s", f.Path(), err)
continue
}
if strings.HasSuffix(path, ".my.cnf") {
_ = f.UseSection("skeema", "client", "mysql") // safe to ignore error (doesn't matter if section doesn't exist)
} else {
_ = f.UseSection(cfg.Get("environment")) // safe to ignore error (doesn't matter if section doesn't exist)
}
cfg.AddSource(f)
cfg.AddSource(f)
}
}
// The host and schema options are special -- most commands only expect
18 dir.go
@@ -8,6 +8,7 @@ import (
"os"
"path"
"path/filepath"
"sort"
"strconv"
"strings"
"time"
@@ -80,11 +81,11 @@ func (dir *Dir) CreateIfMissing() (created bool, err error) {
return false, nil
}
if !os.IsNotExist(err) {
return false, fmt.Errorf("Unable to use directory %s: %s\n", dir.Path, err)
return false, fmt.Errorf("Unable to use directory %s: %s", dir.Path, err)
}
err = os.MkdirAll(dir.Path, 0777)
if err != nil {
return false, fmt.Errorf("Unable to create directory %s: %s\n", dir.Path, err)
return false, fmt.Errorf("Unable to create directory %s: %s", dir.Path, err)
}
return true, nil
}
@@ -107,9 +108,11 @@ func (dir *Dir) HasFile(name string) bool {
return (err == nil)
}
// HasOptionFile returns true if the directory contains a .skeema option file.
// HasOptionFile returns true if the directory contains a .skeema option file
// and the dir isn't hidden. (We do not parse .skeema in hidden directories,
// to avoid issues with SCM metadata.)
func (dir *Dir) HasOptionFile() bool {
return dir.HasFile(".skeema")
return dir.HasFile(".skeema") && dir.BaseName()[0] != '.'
}
// HasHost returns true if the "host" option has been defined in this dir's
@@ -279,6 +282,9 @@ func (dir *Dir) SchemaNames(instance *tengo.Instance) ([]string, error) {
for name := range schemasByName {
schemaNames = append(schemaNames, name)
}
// Schema name list must be sorted so that generateTargetsForDir with
// firstOnly==true consistently grabs the alphabetically first schema
sort.Strings(schemaNames)
return schemaNames, nil
}
@@ -371,8 +377,8 @@ func (dir *Dir) SQLFiles() ([]*SQLFile, error) {
// Subdirs returns a slice of direct subdirectories of the current dir. An
// error will be returned if there are problems reading the directory list.
// If the subdirectory has an option file, it will be read and parsed, with
// any errors in either step proving fatal.
// If the subdirectory has an option file (and it isn't a hidden dir), it will
// be read and parsed, with any errors in either step proving fatal.
func (dir *Dir) Subdirs() ([]*Dir, error) {
fileInfos, err := ioutil.ReadDir(dir.Path)
if err != nil {
40 exit.go
@@ -48,27 +48,33 @@ func (ev *ExitValue) Error() string {
return ev.message
}
// Exit terminates the program. If a non-nil err is supplied, and its Error
// method returns a non-empty string, it will be logged to STDERR. If err is
// an ExitValue, its Code will be used for the program's exit code. Otherwise,
// if err is nil, exit code 0 will be used; if non-nil then exit code 2.
// ExitCode returns an exit code corresponding to the supplied error. If err
// is nil, code 0 (success) is returned. If err is an *ExitValue, its Code is
// returned. Otherwise, exit 2 code (fatal error) is returned.
func ExitCode(err error) int {
if err == nil {
return CodeSuccess
} else if ev, ok := err.(*ExitValue); ok {
return ev.Code
}
return CodeFatalError
}
// Exit terminates the program with the appropriate exit code and log output.
func Exit(err error) {
exitCode := ExitCode(err)
if err == nil {
log.Debug("Exit code 0 (SUCCESS)")
os.Exit(0)
}
exitCode := CodeFatalError
if ev, ok := err.(*ExitValue); ok {
exitCode = ev.Code
}
message := err.Error()
if message != "" {
if exitCode >= CodeFatalError {
log.Error(message)
} else {
log.Warn(message)
} else {
message := err.Error()
if message != "" {
if exitCode >= CodeFatalError {
log.Error(message)
} else {
log.Warn(message)
}
}
log.Debugf("Exit code %d", exitCode)
}
log.Debugf("Exit code %d", exitCode)
os.Exit(exitCode)
}
@@ -173,7 +173,7 @@ func NewInterpolatedShellOut(command string, dir *Dir, extra map[string]string)
resultWithoutPassword := varPlaceholder.ReplaceAllStringFunc(command, replacer)
return NewShellOut(result, resultWithoutPassword), err
}
return NewShellOut(result, result), err
return NewShellOut(result, ""), err
}
// escapeVarValue takes a string, and wraps it in single-quotes so that it will
@@ -6,6 +6,22 @@ import (
"testing"
)
func TestShellOutRun(t *testing.T) {
assertResult := func(command string, expectSuccess bool) {
t.Helper()
s := NewShellOut(command, "")
if err := s.Run(); expectSuccess && err != nil {
t.Errorf("Expected command `%s` to return no error, but it returned error %s", command, err)
} else if !expectSuccess && err == nil {
t.Errorf("Expected command `%s` to return an error, but it did not", command)
}
}
assertResult("", false)
assertResult("false", false)
assertResult("/does/not/exist", false)
assertResult("true", true)
}
func TestRunCaptureSplit(t *testing.T) {
assertResult := func(command string, expectedTokens ...string) {
s := NewShellOut(command, "")
@@ -25,6 +41,16 @@ func TestRunCaptureSplit(t *testing.T) {
assertResult(`/usr/bin/printf ',,,commas, have the,,next priority, if no newlines'`, "commas", "have the", "next priority", "if no newlines")
assertResult("/usr/bin/printf 'spaces work if no other delimiters'", "spaces", "work", "if", "no", "other", "delimiters")
assertResult(`/usr/bin/printf 'intentionally "no support" for quotes'`, "intentionally", `"no`, `support"`, "for", "quotes")
// Test error responses
s := NewShellOut("", "")
if _, err := s.RunCaptureSplit(); err == nil {
t.Error("Expected empty shellout to error, but it did not")
}
s = NewShellOut("false", "")
if _, err := s.RunCaptureSplit(); err == nil {
t.Error("Expected non-zero exit code from shellout to error, but it did not")
}
}
func TestNewInterpolatedShellOut(t *testing.T) {
@@ -24,14 +24,11 @@ func main() {
var cfg *mybase.Config
defer func() {
if err := recover(); err != nil {
if cfg == nil || !cfg.GetBool("debug") {
Exit(NewExitValue(CodeFatalError, fmt.Sprint(err)))
} else {
log.Error(err)
if iface := recover(); iface != nil {
if cfg != nil && cfg.GetBool("debug") {
log.Debug(string(debug.Stack()))
Exit(NewExitValue(CodeFatalError, ""))
}
Exit(NewExitValue(CodeFatalError, fmt.Sprint(iface)))
}
}()
@@ -18,8 +18,10 @@ import (
// [4] is any text after the table body -- we ignore this
var reParseCreate = regexp.MustCompile(`(?is)^(.*)\s*create\s+table\s+(?:if\s+not\s+exists\s+)?` + "`?([^\\s`]+)`?" + `\s+([^;]+);?\s*(.*)$`)
// We disallow CREATE TABLE SELECT and CREATE TABLE LIKE expressions
var reBodyDisallowed = regexp.MustCompile(`(?i)^(as\s+select|select|like|[(]\s+like)`)
// We attempt to disallow CREATE TABLE SELECT and CREATE TABLE LIKE expressions,
// although this is not a comprehensive check by any means, as that would
// require a full SQL DDL parser
var reBodyDisallowed = regexp.MustCompile(`(?i)^(as\s+select|select|like|[(]\s*like)`)
// MaxSQLFileSize specifies the largest SQL file that is considered valid;
// we assume legit CREATE TABLE statements should always be under 16KB.
@@ -94,6 +96,9 @@ func (sf *SQLFile) Delete() error {
// It is the caller's responsibility to populate sf.Contents prior to calling
// this method.
func (sf *SQLFile) validateContents() error {
sf.Error = nil
sf.Warnings = nil
if len(sf.Contents) > MaxSQLFileSize {
sf.Error = fmt.Errorf("%s: file is too large; size of %d bytes exceeds max of %d bytes", sf.Path(), len(sf.Contents), MaxSQLFileSize)
return sf.Error

0 comments on commit a670408

Please sign in to comment.