search

heroku / cli

Heroku CLI
https://devcenter.heroku.com/articles/heroku-cli
ISC License
851 stars 223 forks source link

CLI v9: breaking regression for pg:backups:schedule when passing in timezone #2959

Closed mble-sfdc closed 1 month ago

mble-sfdc commented 2 months ago

This project is for the Heroku CLI only and issues are reviewed as we are able. If you need more immediate assistance or help with anything not specific to the CLI itself, please use https://help.heroku.com.

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

As per https://devcenter.heroku.com/articles/heroku-postgres-backups#set-up-a-backup-schedule, we should be able to pass in an IANA timezone identifier e.g.

$ heroku pg:backups:schedule DATABASE_URL --at '02:00 America/Los_Angeles' --app example-app

On CLI v9, this returns the following:

$ heroku version
heroku/9.0.0 darwin-arm64 node-v16.20.2

$ heroku pg:backups:schedule DATABASE_URL --at "02:00 America/Los_Angeles" -a example-app
 ›   Warning: Unknown timezone America/Los_Angeles. Defaulting to UTC.
 ›   Warning: Continuous protection is already enabled for this database. Logical backups of large databases are likely to fail.
 ›   Warning: See https://devcenter.heroku.com/articles/heroku-postgres-data-safety-and-continuous-protection#physical-backups-on-heroku-postgres.
Scheduling automatic daily backups of postgresql-dimensional-12345 at 02:00 UTC... done

This is due to the restricted handling in parseDate: https://github.com/heroku/cli/blob/v9.0.0/packages/cli/src/commands/pg/backups/schedule.ts#L59-L73

In CLI v8, this was more permissive: https://github.com/heroku/cli/blob/v8.10.0/packages/pg-v5/commands/backups/schedule.js#L21C1-L27C2

What is the expected behavior?

A valid IANA timezone identifier or shorthand is valid for passing into pg:backups:schedule, as it was in CLI v8.

mble-sfdc commented 2 months ago

The current list of valid TZs in https://github.com/heroku/cli/blob/v9.0.0/packages/cli/src/commands/pg/backups/schedule.ts#L24-L38 is very limited - only covering UTC-8 to UTC+1

k80bowman commented 2 months ago

Internal work item

Thank you for reporting, we'll take a look.

k80bowman commented 1 month ago

A fix for this was released in v9.2.0 of the CLI. I'm going to close this issue now, but if it continues to be a problem please feel free to reopen.