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
many: switch to use http numeric statuses as agreed #3486
Conversation
…convert 401 into please buy, atm is just confusing
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.
thank you!
@@ -698,15 +700,15 @@ func (t *remoteRepoTestSuite) TestActualDownloadNonPurchased401(c *C) { | |||
var buf bytes.Buffer | |||
err := download(context.TODO(), "foo", "sha3", mockServer.URL, nil, theStore, nopeSeeker{&buf}, -1, nil) | |||
c.Assert(err, NotNil) | |||
c.Check(err.Error(), Equals, "Please buy foo before installing it.") | |||
c.Check(err.Error(), Equals, "please buy foo before installing it.") |
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.
nice drive-by there
run-checks
Outdated
echo 'Check for usages of http.Status*' | ||
got="" | ||
for dir in $(go list -f '{{.Dir}}' ./... | grep -v '/vendor/' ); do | ||
s="$(grep -nE "http\.Status" "$dir"/*.go |grep -vE "http\.StatusText\(" || true)" |
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.
you could also write that grep -nP 'http\.Status(?!Text)'
, fwiw.
store/store_test.go
Outdated
n := 0 | ||
mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
n++ | ||
w.WriteHeader(http.StatusUnauthorized) | ||
// XXX: the server doesn't behave correctly ATM | ||
// but 401 for non purchases is the unlikely case so far |
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.
is this still true? I thought work was being done on this front
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.
yes, is still true to some extent
Codecov Report
@@ Coverage Diff @@
## master #3486 +/- ##
=======================================
Coverage 77.16% 77.16%
=======================================
Files 373 373
Lines 25793 25793
=======================================
Hits 19903 19903
Misses 4133 4133
Partials 1757 1757
Continue to review full report at Codecov.
|
Also change the code that was interpreting a 401 as "please buy" which is the unlikely case.
Also have a static check to make sure we continue with this style.