app name and id directive limits #71
Conversation
cmd/klotho/main.go
Outdated
| } | ||
| match, err := regexp.MatchString(`^[A-Za-z0-9-_]+$`, cfg.appName) |
There was a problem hiding this comment.
nit: ^[\w-]+$ would be a bit more concise
There was a problem hiding this comment.
i can change it. Also do we think 20 characters is too small for these?
There was a problem hiding this comment.
That's kind of what I was getting at with having to find a pretty restrictive intersection of all the constraints. I think (a) 20 feels on the small side from a usability perspective, but also (b) 20 feels about right from a useful-as-a-length-constraint perspective.
🤷
I guess we can put it in and see if people complain. Maybe also add a log line when it happens, so we can get metrics on how often that is?
There was a problem hiding this comment.
the only issue with a log line is then its visible to the user in both an error sense and logging sense right? The only method we have of streaming back to data dog is through the zap logger.
There was a problem hiding this comment.
Oh hm, actually, the fact that there's an error will already get reported back. Let me check if that contains the error message; if so, we shouldn't do that (since it's potentially sensitive), but it'd be good to include it as a sanitized field (maybe a new kind called "Silent" that gets ignored in the stderr output?).
There was a problem hiding this comment.
Okay yeah, we don't send the log line (whew!). So, I would:
- create a new struct in
logging/fields.gocalledsilentAnalytics { key string, data any }or similar , along with aSilentAnalyticsfunction (you can look atannotationFieldfor an example) - add an instance of that to this log line
- I think you don't need to do anything else; it'll get reported in analytics, but won't get logged extra to console
There was a problem hiding this comment.
I would like to have ., :, and maybe / added to the allowable characters as well as those are common separators.
There was a problem hiding this comment.
We can also move the length checking to the regexp ^[\w-.:/]{3,20}$. And if you compile it first, you can use the compiled .String() to print its regex in the error message.
There was a problem hiding this comment.
We can also move the length checking to the regexp
^[\w-.:/]{3,20}$
I like it as it is now, because it gives a more precise indication of the problem. That is, "can only contain this set of chars" and "has to be 3-20 chars long" are separate enough that I'd prefer (as a user) to be told which of them I'm violating, even though it's true that a single regex can check both.
(I won't fight hard for either option, just providing input.)
There was a problem hiding this comment.
added the characters requested and added an analytics Error. Verified in datadog we see
[User: [email protected]] Klotho parameter check failed. 'app' can only contain alphanumeric, -, _, ., :, and /.
pkg/annotation/capability.go
Outdated
| if len(id) > 20 { | ||
| return cap, fmt.Errorf("'id' must be less than 20 characters in length. 'id' was %s", id) | ||
| } | ||
| match, err := regexp.MatchString(`^[A-Za-z0-9-_]+$`, id) |
|
• Does any part of it require special attention?
• Does it relate to or fix any issue? closes #31 and closes #32
allowing max length of 20 and all AlphaNumeric Characters, - and _. (Can easily change this)
Was going to also make sure Id is set, but that breaks almost every single one of our unit tests, so that would make this a much longer task. Can do it if we want though to make sure that plugins dont have to check that each and every time.
Standard checks