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

fix: don't fail command on failed version check && use rustls #624

Merged
merged 2 commits into from
Jun 22, 2021

Conversation

EverlastingBugstopper
Copy link
Contributor

This PR switches Rover from using openssl to rustls, which I thought it was already doing. I believe this will also fix the underlying issue in #609, meaning folks will not need to install ca-certificates to execute Rover.

Additionally, Rover will no longer stop executing if it fails to check for an updated version, it will simply ignore the failure and continue with the command execution.

@EverlastingBugstopper
Copy link
Contributor Author

@lrlna I fiddled around a bit with trying to remove the extra self.get_rover_config() call that we make in update since it essentially doubles the amount of filesystem calls Rover is making but.. couldn't quite come up with an elegant solution.

Don't think we have to worry about it so much right now but if there's something obvious we could do here I'd love to hear about it

@lrlna
Copy link
Member

lrlna commented Jun 22, 2021

@EverlastingBugstopper the most straightforward solution would be to have rover config to be stored on the rover struct and initialised when the initial Rover instance is started up. Then get_rover_config simply becomes a getter method, and accessing that property on the Rover struct does not do any additional filesystem calls. Similar can be done with client config, although that does seem to be run only once per command, so perhaps it doesn't matter so much.

Up to you if you'd like to implement this sort of change! It's also not a big deal if it stays the way it is right now.

@EverlastingBugstopper
Copy link
Contributor Author

yeah i had considered that, but the problem is that not every command needs access to each one of these and I'd rather not return an API key error if that command doesn't need to talk to the API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants