Closed sighphyre closed 1 year ago
I'm not sure we need to update our MSRV: MSRV is transitive, and unless we explicitly depend on the reqwest version that needs a newer MSRV, we don't actually have a newer MSRV.
This is a good point ☝🏼 I read it as "we do explicitly depend on a version of reqwest" that does this (and that bumping MSRV was just missed previously 🤷🏼). But I'm assuming that this PR was raised because of an issue somewhere? That we saw some problems that this would fix? @sighphyre?
I'm not sure we need to update our MSRV: MSRV is transitive, and unless we explicitly depend on the reqwest version that needs a newer MSRV, we don't actually have a newer MSRV.
Oh, yeah, you're 100% correct here.
I'd bumped this because our build is failing. Without a Cargo lock what is our plan for keeping the build functioning?
Sorry for not responding earlier! please do ping me on slack when things are urgent.
So, breaking things apart.
1) we can use a Cargo.lock to help make tests reliable absolutely - while its considered somewhat bad form to check in lockfiles for libs, I find it useful, as long as folk aren't confused about the implications.
2) we can use a build where we disable the built in reqwest feature to checkout our MSRV. The HTTP clients are all optional.
Putting this together, I think the PR we need does the following:
Closing this after the changes discussed and merged in #58.
This is now required by our reqwest client