-
Notifications
You must be signed in to change notification settings - Fork 230
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.
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
Add scheduled machines to fly.toml #3640
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for taking the time to add support for schedules to fly.toml. I wonder if you have the willing to finish this PR considering the feedback I am about to throw at you :)
fly.toml (aka appconfig) and its translation to machine config is one of the best unit tested part of flyctl and we would like to keep it that way because it also at the core of all flyctl operations.
that said, see how to modify and add new tests starting with:
- https://github.com/superfly/flyctl/blob/master/internal/appconfig/testdata/full-reference.toml
- https://github.com/superfly/flyctl/blob/master/internal/appconfig/machines_test.go
- https://github.com/superfly/flyctl/blob/master/internal/appconfig/serde_test.go
thanks!
Oops, I thought |
@dangra I added tests. one issue: there is one failing one that checks the schedule field of a non-scheduled machine is |
the '24/7' value is a placeholder, the idea is that any machine config field that isn't directly configurable from fly.toml must remain unchanged when updating an existing machine with that field set. Your change is breaking this assumption because it makes Before, for an application to set a machine schedule, it has to That said, please remove the |
in that case, I would probably be worried that people with existing scheduled machines are going to have their schedules removed. should there be a warning printed in that case? I will add the new test later. |
This is a valid concern, I did a quick check on existing machines and there are a few (20 something) that relies on the old behavior from keep working. there is a way to know when not to reset the machine Schedule is by checking the flyctl's version set as machine metadata. The metadata key we are looking for is Point is, after we detect the flyctl version is pre the version your patch is going in, another metadata key should be added so following makes sense? |
Needed this for something, thought I'd throw it upstream.