pkg/stringid: deprecate, move to daemon, and provide copy in client#50504
pkg/stringid: deprecate, move to daemon, and provide copy in client#50504thaJeztah merged 1 commit intomoby:masterfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
852ef2f to
8b997ad
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Rebased and now moved the copy in the daemon to |
The stringid package is used in many places; while it's trivial to implement a similar utility, let's just provide it as a utility package in the client, removing the daemon-specific logic. For integration tests, I opted to use the implementation in the client, as those should not ideally not make assumptions about the daemon implementation. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
|
||
| // GenerateRandomID returns a unique, 64-character ID consisting of a-z, 0-9. | ||
| func GenerateRandomID() string { | ||
| b := make([]byte, 32) |
There was a problem hiding this comment.
Should 32 be fullLen/2 ... or, move the const to the unit test?
(fullLen was also unused in the original, apart from in the unit test.)
There was a problem hiding this comment.
Ha! Perhaps, yes. I didn't pay too close attention to the existing implementation. I assume we won't change the 64-char length, so I'll leave that one for a future exercise (there's a test validating the length).
austinvazquez
left a comment
There was a problem hiding this comment.
Sometimes when making copies it's useful to have a mechanism for detecting drift, but in this case the packages are simple and mature enough that probably that would be over engineering. Continue to monitor and see how it shakes out.
Yeah, in the end, they serve different purposes, or at least, should not have to follow the same contract. For the daemon, there's special handling for all-numeric IDs, because the short-ID can be used a default hostname inside the container (which doesn't like numeric hostnames). For the client, and elsewhere, it was really a convenience utility for presentation (and the randomID proved to be popular for those "I need something random"). That last part... really shouldn't be "us" providing it, but it's 5 lines of code to maintain, so I'm not too upset to include it. |
|
I'll bring this one in 👍 |
pkg/stringid: move to daemon, and provide copy in client
The stringid package is used in many places; while it's trivial
to implement a similar utility, let's just provide it as a utility
package in the client, removing the daemon-specific logic.
For integration tests, I opted to use the implementation in the
client, as those should not ideally not make assumptions about
the daemon implementation.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)