Updated schema tool to work with full semantic versions, i.e., MAJOR.MINOR.PATCH #2417
Conversation
tools/common/schema/updatetask.go
Outdated
|
|
||
| if hasEndVer && cmpVersion(startVer, endVer) > 0 { | ||
| return nil, fmt.Errorf("startVer (%v) must be less than or equal to endVer (%v)", startVer, endVer) | ||
| if startVersionExclusive.Compare(*endVersionInclusive) >= 0 { |
There was a problem hiding this comment.
needs to check when there is no schema update, this won't fail any pipeline (in case they call this using same start/end ver).
| ans, err = readSchemaDir(s.versionsDir, "10.2", "", s.logger) | ||
| s.NoError(err) | ||
| s.Equal(0, len(ans)) |
There was a problem hiding this comment.
Both test cases with envVer="" return empty result. We need to add one more case to cover envVer="" and result is non-empty, like startVer=v2.5, endVer="".
common/log/test_logger.go
Outdated
| t *testing.T | ||
| } | ||
|
|
||
| func (l testLogger) Debug(msg string, tags ...tag.Tag) { |
There was a problem hiding this comment.
(l testLogger) -> (l *testLogger)
There was a problem hiding this comment.
btw, why not using this?
https://github.com/temporalio/temporal/blob/v1.14.3/common/log/zap_logger.go#L57
There was a problem hiding this comment.
not related to this pr, but shall we use https://pkg.go.dev/go.uber.org/zap/zaptest#NewLogger for test logger?
There was a problem hiding this comment.
I was not aware of zaptest.NewLogger. That's kind of what I was looking for, but I need a temporal log.Logger, not a zap.Logger. It seems like there should be a way to wrap a zap.Logger as a log.Logger, but I'm not seeing it.
There was a problem hiding this comment.
D'oh. NewZapLogger. I'm good now.
tools/common/schema/updatetask.go
Outdated
| @@ -80,6 +77,7 @@ const ( | |||
|
|
|||
| var ( | |||
| whitelistedCQLPrefixes = [4]string{"CREATE", "ALTER", "INSERT", "DROP"} | |||
| versionDirectoryRegex = regexp.MustCompile(`^v[\d.]+`) | |||
There was a problem hiding this comment.
does this also match v.3 ?
There was a problem hiding this comment.
It does, but it would fail to parse as a semver. The only reason that we need this regex is to enforce the 'v' prefix. Otherwise we should just leave parsing up to the semver library.
No description provided.