-
Notifications
You must be signed in to change notification settings - Fork 345
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
Updated to Operator SDK 0.5.0 #273
Updated to Operator SDK 0.5.0 #273
Conversation
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
==========================================
- Coverage 89.02% 88.96% -0.06%
==========================================
Files 70 71 +1
Lines 3089 3091 +2
==========================================
Hits 2750 2750
- Misses 226 228 +2
Partials 113 113
Continue to review full report at Codecov.
|
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 - although next time doing a big dependency upgrade, would be better to commit the vendor changes in a separate commit to the code changes 😄
@@ -18,7 +18,7 @@ spec: | |||
- name: jaeger-operator | |||
image: jaegertracing/jaeger-operator:1.10.0 | |||
ports: | |||
- containerPort: 60000 | |||
- containerPort: 8383 |
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.
Does this port have any particular significance? Would it be better to have something in the same ballpark as other Jaeger ports?
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.
The port 60000 didn't seem to be used and is probably a left-over from a copy/paste or generated code.
The port 8383 is the one used in newly created operators.
@jpkrohling could you please make sure that self-provisioning of ES cluster works? You mentioned that this one generates more one additional file and ES files contains only one generated file. |
This really needs an e2e test... It's on your queue, right? The ES operator is currently not working for me, but seems to be something unrelated to this PR:
|
Great idea, will do that! |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
efb07d8
to
f809a3c
Compare
I'm merging, as @pavolloffay tested the self-provision and it works. |
Closes #272
Signed-off-by: Juraci Paixão Kröhling [email protected]