Skip to content

Allow the ability to pass timezone when generating migration files #509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 24, 2021
Merged

Allow the ability to pass timezone when generating migration files #509

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions internal/cli/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ import (

const (
defaultTimeFormat = "20060102150405"
createUsage = `create [-ext E] [-dir D] [-seq] [-digits N] [-format] NAME
defaultTimezone = "UTC"
createUsage = `create [-ext E] [-dir D] [-seq] [-digits N] [-format] [-tz] NAME
Create a set of timestamped up/down migrations titled NAME, in directory D with extension E.
Use -seq option to generate sequential up/down migrations with N digits.
Use -format option to specify a Go time format string. Note: migrations with the same time cause "duplicate migration version" error.
Use -format option to specify a Go time format string. Note: migrations with the same time cause "duplicate migration version" error.
Use -tz option to specify the timezone that will be used when generating non-sequential migrations (defaults: UTC).
`
gotoUsage = `goto V Migrate to version V`
upUsage = `up [N] Apply all or N up migrations`
Expand Down Expand Up @@ -162,6 +164,7 @@ Database drivers: `+strings.Join(database.List(), ", ")+"\n", createUsage, gotoU
extPtr := createFlagSet.String("ext", "", "File extension")
dirPtr := createFlagSet.String("dir", "", "Directory to place file in (default: current working directory)")
formatPtr := createFlagSet.String("format", defaultTimeFormat, `The Go time format string to use. If the string "unix" or "unixNano" is specified, then the seconds or nanoseconds since January 1, 1970 UTC respectively will be used. Caution, due to the behavior of time.Time.Format(), invalid format strings will not error`)
timezoneName := createFlagSet.String("tz", defaultTimezone, `The timezone that will be used for generating timestamps (default: utc)`)
createFlagSet.BoolVar(&seq, "seq", seq, "Use sequential numbers instead of timestamps (default: false)")
createFlagSet.IntVar(&seqDigits, "digits", seqDigits, "The number of digits to use in sequences (default: 6)")

Expand All @@ -180,7 +183,12 @@ Database drivers: `+strings.Join(database.List(), ", ")+"\n", createUsage, gotoU
log.fatal("error: -ext flag must be specified")
}

if err := createCmd(*dirPtr, startTime, *formatPtr, name, *extPtr, seq, seqDigits, true); err != nil {
timezone, err := time.LoadLocation(*timezoneName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid loading TZ data if the named tz is the default. That way, missing tz data won't break the CLI if a tz isn't specified.

Copy link
Contributor Author

@nronas nronas Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhui on another thought, I cannot see how this will break, since the default location will be UTC even if you do not specify the timezone. Essentially we will do time.LoadLocation("UTC")

With timezone

go run ./cmd/migrate/... create -tz Europe/Athens -ext postgres hello
/Users/nronas/projects/migrate/20210223204225_hello.up.postgres
/Users/nronas/projects/migrate/20210223204225_hello.down.postgres

Without timezone

go run ./cmd/migrate/... create -ext postgres hello_no_tz
/Users/nronas/projects/migrate/20210223184235_hello_no_tz.up.postgres
/Users/nronas/projects/migrate/20210223184235_hello_no_tz.down.postgres

With unknown timezone

go run ./cmd/migrate/... create -tz Asgard -ext postgres hello_invalid_tz
unknown time zone Asgard
exit status 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is when the tz database is missing, not when the -tz option is not specified or invalid/incorrect.

From the time.LoadLocation() docs:

The time zone database needed by LoadLocation may not be present on all systems, especially non-Unix systems. LoadLocation looks in the directory or uncompressed zip file named by the ZONEINFO environment variable, if any, then looks in known installation locations on Unix systems, and finally looks in $GOROOT/lib/time/zoneinfo.zip.

Copy link
Contributor Author

@nronas nronas Feb 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I completely misinterpreted what you were referring to. I think we should still be fine since zoneinfo does a look upon an IANA file only if the location name is neither 'UTC' nor 'Local' https://github.com/golang/go/blob/master/src/time/zoneinfo.go#L630-L635

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging deeper and discovering that the tz db isn't loaded if the location UTC or Local are specified! Looks like this behavior has been around since at least Go 1.4 and seems unlikely to change in the future, so I think the current approach is fine. The fix is easy enough to make should this break in the future.

if err != nil {
log.fatal(err)
}

if err := createCmd(*dirPtr, startTime.In(timezone), *formatPtr, name, *extPtr, seq, seqDigits, true); err != nil {
log.fatalErr(err)
}

Expand Down