update implementation #1
Conversation
restart.go
Outdated
|
|
||
| package updater | ||
|
|
||
| // Fork process that inherites listening sockets |
updater.go
Outdated
| // New creates a new updater. By default the updater will check for updates every hour | ||
| // but this may be changed by passing Frequency as an option. The minimum | ||
| // frequency is 10 minutes. Anything less than that will cause an error. | ||
| // Supply the WantNotfications option to get data on the state up update operations for |
There was a problem hiding this comment.
Typo in last sentence of this comment? I am a bit confused by this.
updater.go
Outdated
| } | ||
|
|
||
| // WantNotfications pass a function that will collect information about updates. | ||
| func WantNotfications(hnd NotificationHandler) func() interface{} { |
updater.go
Outdated
| } | ||
|
|
||
| func updater(settings tuf.Settings, ticker <-chan time.Time, notifications NotificationHandler) { | ||
| for _ = range ticker { |
There was a problem hiding this comment.
Do we need some mechanism for exiting this loop?
There was a problem hiding this comment.
No, this loop reads from a channel, when the channel is closed, the loop exits which is the desired behavior.
There was a problem hiding this comment.
Where is the channel closed? I see that Stop() is called on the ticker, but according to the docs that does not close the channel.
There was a problem hiding this comment.
Good catch, and I can't close the channel myself because of the direction of the channel which seems like an oversight imho. I'll create something different so we can handle graceful shutdown.
There was a problem hiding this comment.
You don't need to close the channel, and would probably cause a panic if you do.
Calling Stop should be enough to not get any more ticks.
There was a problem hiding this comment.
It is possible that leaving this loop hanging is a reasonable enough strategy, but we should make a conscious decision of whether we need a graceful shutdown here or not.
updater.go
Outdated
| var ErrCheckFrequency = fmt.Errorf("Frequency value must be %q or greater", minimumCheckFrequency) | ||
|
|
||
| // ErrPackageDoesNotExit the package file does not exist | ||
| var ErrPackageDoesNotExit = fmt.Errorf("package file does not exist") |
updater.go
Outdated
| if os.IsNotExist(err) { | ||
| return ErrPackageDoesNotExit | ||
| } | ||
| cmd := exec.Command(updatePackagePath, "-rollback") |
There was a problem hiding this comment.
Will it be possible in the general case for each update package to know how to roll back to the previous version? It seems to me we may want to encode some state checkpointing/rollback into the updater itself, and then each package does not need to consider this.
updater.go
Outdated
| return ErrPackageDoesNotExit | ||
| } | ||
| if err != nil { | ||
| return errors.Wrap(err, "checking for package existance") |
updater.go
Outdated
| // Start begins checking for updates. | ||
| func (u *Updater) Start() { | ||
| u.ticker = time.NewTicker(u.checkFrequency) | ||
| go updater(u.settings, u.ticker.C, u.notificationHandler) |
There was a problem hiding this comment.
I think it would be cleaner if you put the ticker inside the updater function, and pass in a done chan struct{} to start/stop the go-routine.
the updater func would select between the ticker and the done channel, and when it receives done the go-routine can exit cleanly.
There was a problem hiding this comment.
I made a simple example just in case: https://play.golang.org/p/U6apw-52tB
| if err != nil { | ||
| return errors.Wrap(err, "opening role file for writing") | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
should this be above the error handling?
There was a problem hiding this comment.
No. If OpenFile returns an error the file has not been opened, in addition, by convention if an error is returned there is no guarantee that 'f' will not be null so calling f.Close() could create a panic.
| } | ||
|
|
||
| func (r *localRepo) getRole(name role, val interface{}) error { | ||
| err := validateRole(name) |
There was a problem hiding this comment.
Nit: a lot of these blocks could be made more succinct by doing:
if err := validateRole(name); err != nil {
return err
}
This is still a work in progress. I just wanted to give people a preview of the approach and get feedback.