Factor out ingress from all-in-one and query, as common to both but i… #91
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
- Coverage 99.35% 99.35% -0.01%
==========================================
Files 18 18
Lines 777 773 -4
==========================================
- Hits 772 768 -4
Misses 5 5
Continue to review full report at Codecov.
|
|
@jpkrohling After your comment about Could this be because the operator SDK has changed? |
Could be -- are you running the Operator SDK 0.0.7? |
|
I used the instructions in CONTRIBUTING.adoc - so it is based on master. I will try with 0.0.7. |
…s independent of either deployment strategy Signed-off-by: Gary Brown <[email protected]>
|
Yes that worked - I rebased on your generate change and ran the command, but there were no additional changes, which I guess is correct as all it did was move the Ingress spec. |
|
Great, would you mind sending a PR with the change to the CONTRIBUTING? |
|
Will do. |
There was a problem hiding this comment.
Initially, I thought your idea about having the ingress scoped by component was a better approach, especially considering that we have discussed adding another UI in the past, but I think we can keep it simple for now and refactor it later. It shouldn't be hard to keep backward compatibility if we ever need to change this.
I just have a question about the new type QueryIngress, but other than that,
Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)
pkg/ingress/query.go, line 19 at r1 (raw file):
} // NewQueryIngress returns a new ingress object for the Query service
The GoDoc is not accurate anymore. I'm also not sure what advantages we have here in wrapping the logic in the Get() method of a new type, instead of just directly returning an Ingress. Did you have a specific use case in mind?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jpkrohling)
pkg/ingress/query.go, line 19 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
The GoDoc is not accurate anymore. I'm also not sure what advantages we have here in wrapping the logic in the
Get()method of a new type, instead of just directly returning anIngress. Did you have a specific use case in mind?
No, it was just to keep a consistent pattern with the other components being returned. Would you prefer this changed, or should I just update the doc?
Signed-off-by: Gary Brown <[email protected]>
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
…s independent of either deployment strategy
This change was made as currently the NewQueryIngress method takes a
JaegerSpecbut no indication of whether the config is under all-in-one or query.One possibility would be to just supply the
JaegerIngressSpecto this method - but the ingress is a separate component so I think it is better if factored out.Only other question - currently the config is under top level
ingressnode - but the code has been structured based on it being an ingress for the query endpoint. If we expect other service endpoints to be exposed via an ingress, then we might want to have something like:Let me know thoughts on this change, and whether it would be better to remain open to other ingresses.
Signed-off-by: Gary Brown [email protected]