Skip to content
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

Merged

Conversation

jpkrohling
Copy link
Contributor

Closes #272

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

@jpkrohling
Copy link
Contributor Author

This change is Reviewable

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #273 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
pkg/apis/jaegertracing/v1/freeform.go 85.71% <ø> (ø) ⬆️
pkg/apis/jaegertracing/v1/jaeger_types.go 100% <ø> (ø) ⬆️
...ntauthentication/v1alpha1/zz_generated.defaults.go 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 898bb43...f809a3c. Read the comment docs.

Copy link
Contributor

@objectiser objectiser left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@pavolloffay
Copy link
Member

@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.

@jpkrohling
Copy link
Contributor Author

could you please make sure that self-provisioning of ES cluster works?

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:

time="2019-03-07T13:40:34Z" level=error msg="error syncing key (myproject/elasticsearch): Failed to reconcile Roles and RoleBindings for Elasticsearch cluster: failed to create ClusterRole elasticsearch-metrics: clusterroles.rbac.authorization.k8s.io is forbidden: User \"system:serviceaccount:openshift-logging:elasticsearch-operator\" cannot create clusterroles.rbac.authorization.k8s.io at the cluster scope: no RBAC policy matched"

@jpkrohling
Copy link
Contributor Author

would be better to commit the vendor changes in a separate commit to the code changes

Great idea, will do that!

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling
Copy link
Contributor Author

I'm merging, as @pavolloffay tested the self-provision and it works.

@jpkrohling jpkrohling merged commit 8d5f92c into jaegertracing:master Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants