Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
NewShortenedUnitTag #86
Conversation
jameinel
reviewed
Nov 13, 2017
Do we want this to be an explicit different tag type?
I think we'd rather have all units of an application have the longer names internally, and only change it for creating the 'service' object.
| "regexp" | ||
| "strconv" | ||
| "strings" | ||
| ) | ||
| const UnitTagKind = "unit" | ||
| +// minShortenedLength defines minimum size of shortened unit tag, other things depend | ||
| +// on that value so change it carefully. | ||
| +const minShortenedLength = 21 |
jameinel
Nov 13, 2017
Owner
What was the actual constraint we ran into? I thought the actual service limit was closer to 40.
| +// a service name. It uses a hash so the resulting tag should be unique. | ||
| +// It will panic if the given unit name is not valid or if maxLength is less | ||
| +// than minShortenedLength | ||
| +func NewShortenedUnitTag(unitName string, maxLength int) UnitTag { |
jameinel
Nov 13, 2017
Owner
Should this be ShortenedUnitTag or should this be
UnitTag.ShortedId() or something along those lines?
| + idLen := len(id) | ||
| + if idLen < 3 { | ||
| + idLen = 3 | ||
| + } |
jameinel
Nov 13, 2017
Owner
does that include the '/' ?
I think we'd want to go up to /999, though w/ the recent changes we may want to reserve up to /9999
Thoughts?
jameinel
approved these changes
Nov 13, 2017
I can possibly be convinced about the panic(), as I think it is something that names tends to do (I think a lot of the New* functions panic() already).
But it doesn't feel like our use cases are so hard that we couldn't handle an error return.
That would also let us add a check for the edge case being rejected.
| +// It will panic if maxLength is less than minShortenedLength. | ||
| +func (t UnitTag) ShortenedString(maxLength int) string { | ||
| + if maxLength < minShortenedLength { | ||
| + panic(fmt.Sprintf("max length must be at least %d, not %d", minShortenedLength, maxLength)) |
jameinel
Nov 13, 2017
Owner
I just noticed this now. I really dislike panics in production code.
Is it reasonable to change the api to return an error?
panic() as a sign of programming mistake might be ok, but it often happens accidentally when we end up validating user data with them.
wupeka
Nov 13, 2017
Member
I've fixed it here, but changing it in whole juju/names would require .v3
|
nice, lgtm |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju-names |
jujubot
merged commit 54f0084
into
juju:v2
Nov 13, 2017
| + // 8 for hash, 2 for two dashes | ||
| + maxNameLength := maxLength - idLen - len(UnitTagKind) - 8 - 2 | ||
| + if len(name) > maxNameLength { | ||
| + hash := crc32.Checksum([]byte(name), crc32.IEEETable) |
rogpeppe
Nov 13, 2017
Owner
Is CRC32 the right algorithm to be using here? Shouldn't we be using something somewhat more collision-resistant, such as (appropriately truncated) sha256?
Also, this is a bit over-enthusiastic - it truncates to less than the specified length. See https://play.golang.org/p/mJEuAcPxr5 where we try to truncate to 25 characters, but the result is truncated to 22 characters.
wupeka commentedNov 12, 2017
New function NewShortenedUnitTag - create a unit tag which has at most maxLength byte in String() representation, to use e.g. as a service name for systemd.