-
Notifications
You must be signed in to change notification settings - Fork 344
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
Sanitize names that must follow DNS naming rules #483
Sanitize names that must follow DNS naming rules #483
Conversation
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
5c2d466
to
a5d9140
Compare
Codecov Report
@@ Coverage Diff @@
## master #483 +/- ##
==========================================
- Coverage 91.7% 91.68% -0.02%
==========================================
Files 64 65 +1
Lines 3181 3200 +19
==========================================
+ Hits 2917 2934 +17
- Misses 184 185 +1
- Partials 80 81 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #483 +/- ##
==========================================
+ Coverage 91.7% 91.74% +0.04%
==========================================
Files 64 65 +1
Lines 3181 3199 +18
==========================================
+ Hits 2917 2935 +18
Misses 184 184
Partials 80 80
Continue to review full report at Codecov.
|
@jpkrohling We should document this approach on a notable area. Otherwise, the end user will be waiting for a given name, but Jaeger-operator creates a different name (if the name contains invalid chars). |
+1 , but note that we only change service names and volume names, both are what I'd consider implementation details. |
By the way: our readme is already somewhat crowded. I'm actually not sure this belongs to the main documentation. @JStickler, any ideas? |
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.
Lgtm subject to thd suggested changes.
@jpkrohling Yes, the operator README is really long (14 pages if I was going to print it). But it's not part of this PR, and I'm not sure what you were getting at in your question to me? Could you clarify what you wanted my opinion on? |
@JStickler sorry, I kinda forgot to add the context :-) The change from this PR introduces a change that needs to be documented: if the Jaeger IMO, this is too specific to be useful in the readme, but I can imagine that users might be confused when they create a Jaeger instance named |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
I'm merging, but will work on the documentation based on what @JStickler suggests. |
Closes #466 by making the sanitizing names for services and volumes, to be DNS-compliant. Apart from this change, a name such as
jaegerqe.test
is now possible and yields no errors.The bigger topic of "validation" in general isn't being touched by this PR.
cc @jkandasa
Signed-off-by: Juraci Paixão Kröhling [email protected]