Factor out ingress from all-in-one and query, as common to both but i…

[objectiser]

…s independent of either deployment strategy

This change was made as currently the NewQueryIngress method takes a JaegerSpec but no indication of whether the config is under all-in-one or query.

One possibility would be to just supply the JaegerIngressSpec to 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 ingress node - 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:

spec:
  ingress:
    query:
      enabled: true

Let me know thoughts on this change, and whether it would be better to remain open to other ingresses.

Signed-off-by: Gary Brown gary@brownuk.com

[jpkrohling]

This change is 

[codecov]

Codecov Report

Merging #91 into master will decrease coverage by <.01%.
The diff coverage is 100%.

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

Impacted Files	Coverage Δ
pkg/deployment/query.go	100% <ø> (ø)	⬆️
pkg/deployment/all-in-one.go	100% <ø> (ø)	⬆️
pkg/ingress/query.go	100% <100%> (ø)	⬆️
pkg/controller/production.go	100% <100%> (ø)	⬆️
pkg/controller/all-in-one.go	100% <100%> (ø)	⬆️

---

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 17e8480...e885beb. Read the comment docs.

[objectiser]

@jpkrohling After your comment about make generate on the other PR, I tried it on this one - but had to install the operator SDK. Once done I ran make generate but got the following error:

2018/11/06 18:58:07 must run command in project root dir: stat ./build/Dockerfile: no such file or directory
make: *** [Makefile:87: generate] Error 1

Could this be because the operator SDK has changed?

[jpkrohling]

Could this be because the operator SDK has changed?

Could be -- are you running the Operator SDK 0.0.7?

[objectiser]

I used the instructions in CONTRIBUTING.adoc - so it is based on master. I will try with 0.0.7.

[objectiser]

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.

[jpkrohling]

Great, would you mind sending a PR with the change to the CONTRIBUTING?

[objectiser]

Will do.

[jpkrohling]

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?

[objectiser]

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 an Ingress. 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?

[jpkrohling]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved