Skip to content
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

Track CLI version. #257

Merged
merged 6 commits into from
Apr 16, 2021
Merged

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Apr 15, 2021

Problem: When we have an updated configuration the CLI code is updated so it can parse the new configuration schema, but if the CLI is run (this initial run will cause the config data to be cached for 5mins) before the binary is updated to a new version (which includes code that expects new config fields) and if the new CLI version is then subsequently run before the currently cached config TTL has expired, then a user will find the new CLI code will try to read a struct field that doesn't have a value because the underlying configuration data isn't updated.

See also: #258

Solution: The customer waits 5 minutes (or however long the TTL is set to in the config) and then tries the CLI command again and the configuration will be considered stale and subsequently updated before the subcommand is executed.

Better Solution: Implement a check alongside the 'stale config check' that validates if the CLI binary was recently modified (i.e. updated) and if so we'll force the fetching of the configuration.

Notes: this will require an update to the configuration endpoint to include a new version.

@Integralist Integralist added the bug Something isn't working label Apr 15, 2021
@kailan kailan linked an issue Apr 15, 2021 that may be closed by this pull request
pkg/check/check.go Outdated Show resolved Hide resolved
@Integralist
Copy link
Collaborator Author

The implementation currently doesn't work. Discussed with Kailan and am addressing a different way.

@Integralist
Copy link
Collaborator Author

@phamann have reworked the implementation and @kailan has validated it locally too.

cmd/fastly/main.go Outdated Show resolved Hide resolved
@Integralist Integralist requested a review from fgsch April 16, 2021 08:15
Copy link
Member

@fgsch fgsch left a comment

Choose a reason for hiding this comment

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

Lightly tested but LGTM.

Copy link
Member

@kailan kailan left a comment

Choose a reason for hiding this comment

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

Code reviewed and tested locally – works as expected on configurations from 0.26.x. No more requests than needed are made to the config service. Ship it 🚀

@Integralist Integralist merged commit 46d4f0d into master Apr 16, 2021
@Integralist Integralist deleted the integralist/20210415_lastchecked_version branch April 16, 2021 09:37
@Integralist Integralist changed the title Check when binary was updated. Track CLI version. Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERROR: error parsing rustup constraint: improper constraint: .
3 participants