search

cloudfoundry-attic / blockhead

CF-Extensions Blockhead project
Apache License 2.0
3 stars 3 forks source link

Change app credentials #65

Closed swetharepakula closed 6 years ago

swetharepakula commented 6 years ago
cfdreddbot commented 6 years ago

Hey swetharepakula!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

swetharepakula commented 6 years ago

fixes #64

MHBauer commented 6 years ago

I do not see any reasonable expansion at this time.

Multiple addresses for a single service binding seems unreasonable, especially with different port mappings. (Thought here is some type of clustered database where you can talk to any node.)

Do we see protocols being relevant? TCP/UDP? Higher level? HTP vs websocket?

I need a more concrete understanding of how this is given to the end-user app. I think we should aim for simplicity. Address. Only one access point if possible. Given the current specificity to exposing a particular container, I think we're fine.

Good for merge from me. File an issue to tackle it later.

On 10/03/2018 08:09 PM, Swetha Repakula wrote:

@swetharepakula commented on this pull request.

In pkg/containermanager/container_manager.go https://github.com/cloudfoundry-incubator/blockhead/pull/65#discussion_r222526483:

@@ -16,13 +16,13 @@ type BindConfig struct { }

type Binding struct {

  • HostIP string
  • Port string
  • Port string

That is true, we don't really need this to be a struct anymore. It wasn't intentional, but you bring up a good point. Do we think we would need to expand this in the future?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/cloudfoundry-incubator/blockhead/pull/65#discussion_r222526483, or mute the thread https://github.com/notifications/unsubscribe-auth/AMuDAT4zTWIF0WB3XKJVED74IjovLFIVks5uhXv0gaJpZM4XHRLy.