search

Metaswitch / clearwater-docker

Docker integration for Project Clearwater
Other
41 stars 64 forks source link

[Reviewer: Mat] Headless Bono and Ellis services #76

Closed johadalin closed 7 years ago

johadalin commented 7 years ago

This has the minor code changes to make Bono and Ellis the same as the rest of our services. I've updated the README to clarify our mainline expectations. It seemed like a bad plan to lose the information about how things were configured previously, in case someone wants to work on GKE or something in future, so i've created a subsection describing how to configure Bono LoadBalancer and Ellis NodePort services. Worth reading over those to make sure it makes sense.

I've also noticed and fixes up the image link. It wasn't rendering correctly in the github UI, and i think it was just unhappy about the spaces in the file name. Looks good now.

MatMeredith commented 7 years ago

LGTM. Perhaps controversially I've suggested some markups to the README by committing an updated version. You can presumably always just revert the commit if you don't like them ;-)

MatMeredith commented 7 years ago

We should probably also document the HSS option that I added to clearwater-auto-config-docker and change the example additional shared config (now that the HSS can be configured properly)? I can do that separately though if you like as I obviously should have done it previously.

johadalin commented 7 years ago

Cool. Markups as a commit feels reasonable in this case. Just easier in the README really. Only thing is, i assume you've got a whitespace filter on or something? It was a bit annoying to work out where the actual changes were, as git highlights all these blocks where just one space has changed. Otherwise, looks good.

I think it is probably worth documenting the new config separately, just to keep things logically separated. I'll leave it with you for now as you added it, and have been working on that side of things, but pass it back to me if you don't have time. I'm working on the other doc atm.

mdavis-xyz commented 6 years ago

@johadalin : The part of the readme about the Bono LoadBalancer and Ellis NodePort is not clear.

add a line to the "http" port configuration specifying nodePort: <port number>

Hmm?

Like this?

apiVersion: v1
kind: Service
metadata:
  name: ellis
spec:
  type: NodePort
  ports:
  - name: "http"
    port: 80
    nodePort: 80
  selector:
    service: ellis

Is that the right spot? Should I delete the port line? Where did port 30080 come from? Why not just use port 80?