-
Notifications
You must be signed in to change notification settings - Fork 2.1k
cli/command: remove dependency on distribution/uuid #5879
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
Conversation
This uuid package was introduced in 89db01e, but we want to reduce dependency on the old docker/distribution module. Replace it with google/uuid, which is a commonly used module for this and already a dependency. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #5879 +/- ##
==========================================
+ Coverage 59.32% 59.34% +0.02%
==========================================
Files 353 353
Lines 29731 29731
==========================================
+ Hits 17637 17645 +8
+ Misses 11112 11103 -9
- Partials 982 983 +1 |
| // OTEL processors may think the same process is restarting | ||
| // continuously. | ||
| semconv.ServiceInstanceID(uuid.Generate().String()), | ||
| semconv.ServiceInstanceID(uuid.NewString()), |
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.
I should mention that in theory, uuid.NewString() could panic, but I think chances for this to happen are really exceptional, and it's probably fine to just panic in that case;
cli/vendor/github.com/google/uuid/version4.go
Lines 17 to 23 in 3f154ad
| // NewString creates a new random UUID and returns it as a string or panics. | |
| // NewString is equivalent to the expression | |
| // | |
| // uuid.New().String() | |
| func NewString() string { | |
| return Must(NewRandom()).String() | |
| } |
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.
Alternatively we could do NewRandom(), check the error, and ignore it, but not sure if that's good.
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.
Ah, and existing code also could panic, so I think this comment can be ignored, as it doesn't change anything from that perspective;
| // Any other errors represent a system problem. What did someone | |
| // do to /dev/urandom? | |
| panic(fmt.Errorf("error reading random number generator, retried for %v: %v", totalBackoff.String(), err)) |
This uuid package was introduced in 89db01e (#4889), but we want to reduce dependency on the old docker/distribution module.
Replace it with google/uuid, which is a commonly used module for this and already a dependency.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)