ulid for instance id #304
ulid for instance id #304
Conversation
Signed-off-by: Aaron Sutula <[email protected]>
| return len(instanceID) > 0 | ||
| // _, err := uuid.Parse(instanceID) | ||
| // return err == nil |
There was a problem hiding this comment.
We can always add more to this in the future, but this is the only check needed for now since we're allowing clients to specify any string as an id.
db/collection.go
Outdated
| partial := &struct{ ID *string }{} | ||
| if err := json.Unmarshal(t, partial); err != nil { | ||
| log.Fatalf("error when unmarshaling json instance: %v", err) | ||
| return core.EmptyInstanceID, err |
There was a problem hiding this comment.
log.Fatal seems strange to me. I'm not sure why we'd ever want to actually just kill the process when there is an error we can return to the caller. I made this change here since I was working around/in this code, but this log.Fatal is used other places as well. If there is a good reason to revert this, let me know.
There was a problem hiding this comment.
There are cases where fatal makes sense. For example, "I've encountered an error that indicates a fatal flaw on logic and I should not be used anymore". There are a number of places where fatal is used like that in here. This one around getting the instance ID could be like that. The system allowed the ID to enter, if it can't get it back out, something bad happened that can't be recovered from. It's nice to have a distinction in the code the separates these kinds of things. But, I'm not super partial to one direction for this one.
There was a problem hiding this comment.
In those cases where you're saying log.Fatal is the way to go, what is that experience like on the user/client end?
For the sake of simplicity, I'll change this one back.
There was a problem hiding this comment.
It would kill their daemon, hopefully they would report the fatal bug.
|
Not sure what's up with the tests. There is a folder sync test failing because the client trees aren't equal, but they look perfectly equal in the log output. And it passes for me locally 😞 |
Signed-off-by: Aaron Sutula <[email protected]>
Signed-off-by: Aaron Sutula <[email protected]>
Signed-off-by: Aaron Sutula <[email protected]>
Signed-off-by: Aaron Sutula <[email protected]>
Signed-off-by: Aaron Sutula <[email protected]>
This reverts commit f3a8c66. Signed-off-by: Aaron Sutula <[email protected]>
asutula
force-pushed
the
asutula/ulid-ids
branch
from
7ec398d
to
349a098
Compare
Signed-off-by: Aaron Sutula <[email protected]>
Signed-off-by: Aaron Sutula <[email protected]>
Pretty simple change since we're allowing clients to use any string id if they want.
closes #280