updated implementation to comply with S3 Read/Write API endpoint spec. #14
Conversation
|
Ignore the semaphoreci thing, it was a 10% day thing that I didn't realise hooked in |
service/handlers_test.go
Outdated
| @@ -86,11 +88,47 @@ func TestAddAdminHandlers(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestMethodHandlerAcceptsNonEmptyResourcePath(t *testing.T) { | |||
| r := mux.NewRouter() | |||
There was a problem hiding this comment.
Having re-read these tests, I think that they're testing the same as the other tests unnecessarily. I think going down the named routes route would actually be better to test the setting of the resource path.
r.Get("mh").URL("uuid", <uuid>) == /notempty/<uuid>
There was a problem hiding this comment.
I don't mind changing but I am worried whether there will be a quorum? Other reviewers may think otherwise...
There was a problem hiding this comment.
You could take out the additional tests and see if coverage goes down if you aren't convinced. Also Pete just told me that you can see inline the coverage if you use Atom as an editor - I am going to check it out now :)
| servicesRouter.Handle(fmt.Sprintf("%s%s", resourcePath, "/{uuid:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}}"), mh) | ||
| servicesRouter.Handle(fmt.Sprintf("%s%s", resourcePath, "/__count"), ch) | ||
| servicesRouter.Handle(fmt.Sprintf("%s%s", resourcePath, "/__ids"), ih) | ||
| servicesRouter.Handle(fmt.Sprintf("%s%s", resourcePath, "/"), ah) |
There was a problem hiding this comment.
Missing test for /__count, /__ids and /.
service/processor.go
Outdated
| return nil | ||
| } | ||
| func addTransactionID(params *s3.PutObjectInput, tid string) { |
There was a problem hiding this comment.
I think having this as a separate function is unnecessary, it just seems to be affecting readability.
There was a problem hiding this comment.
I can change it but your suggestion incourages bad coding practices as a lot of our go code does.
This has to do with excessive in-lining and dealing with more than one responsibility in a function/method, all of which increases its cyclomatic complexity. Some say the function should fit in your palm, others - it should be no more that the screen length. There is a lot of literature about it - "Code Complete", Object Calystenics, articles on refactoring by ThoughtWorks etc
service/processor.go
Outdated
|
|
||
| func readerServiceUnavailable(uuid string, err error, rw http.ResponseWriter) { | ||
| if uuid != "" { | ||
| log.Errorf("Error from reader: '%v': %v", uuid, err.Error()) |
There was a problem hiding this comment.
It'd be nice if this indicated that it was the UUID being displayed, rather than just listing the values. It also doesn't need to be a %v, it should be %s as UUID is only a string.
service/processor.go
Outdated
| if uuid != "" { | ||
| log.Errorf("Error from reader: '%v': %v", uuid, err.Error()) | ||
| } else { | ||
| log.Errorf("Error from reader: %v", err.Error()) |
There was a problem hiding this comment.
I disagree with splitting the errors, it's useful to know about the absence of the UUID.
There was a problem hiding this comment.
agreed to change uuid to Request URI when logging errors
service/processor_test.go
Outdated
| err = w.Write(expectedUUID, &p, ct, expectedTransactionId) | ||
| assert.NoError(t, err) | ||
| assert.NotEmpty(t, s.putObjectInput) | ||
| assert.Equal(t, expectedTransactionId, *s.putObjectInput.Metadata[transactionid.TransactionIDKey]) |
There was a problem hiding this comment.
Why can't this assert just be added into TestWritingToS3? This is just duplicating that test.
There was a problem hiding this comment.
They shouldn't be the same. These tests test specifically that that transaction ID is passed to S3. I have re-written them in a better way. TestWritingToS3 is already testing more than it should. There used to be a rule that you have one assertion per unit test, but again with our "new Go coding standards" this is no longer the case.
…neric-rw-s3 into concept_store_support
No description provided.