Add ocdev link command
#354
Conversation
|
Fix #342 |
kadel
added
the
needs-rebase
pkg/component/component.go
Outdated
| // GetComponentService returns the Service object associated with the given | ||
| // component. | ||
| // An error is thrown when exactly one Service is not found for the component. | ||
| func GetComponentService(client *occlient.Client, componentName string, applicationName string) (*corev1.Service, error) { |
There was a problem hiding this comment.
I'm not sure that this function belongs here. If you really want to keep it here than it definitely shouldn't be public. Otherwise, it is a potential risk for creating cyclic imports like in the storage :-(
It might be better to do it similarly like here: f2d5b10#diff-6025e9b620e9321c03c11bf29e5c4dbdR1190
There was a problem hiding this comment.
moved this similar to the current deployment config logic
pkg/component/component.go
Outdated
| // LinkInfo contains the information about the link being created between | ||
| // components | ||
| type LinkInfo struct { | ||
| ComponentName string |
There was a problem hiding this comment.
Is this source or target component name?
Why is there only one?
There should be either comment, or better variable name should specify that.
There was a problem hiding this comment.
added source and target components
pkg/component/component.go
Outdated
| } | ||
|
|
||
| return &LinkInfo{ | ||
| ComponentName: sourceComponent, |
There was a problem hiding this comment.
Why are we returning just sourceComponent?
There was a problem hiding this comment.
modified LinkInfo to include both source and target components
cmd/link.go
Outdated
| printed to STDOUT. | ||
| `, | ||
| Example: ` | ||
| # Link current component to a component 'nodejs' |
There was a problem hiding this comment.
There should be just two spaces at the beginning of each line, to make it properly align
cmd/link.go
Outdated
| `, | ||
| Example: ` | ||
| # Link current component to a component 'nodejs' | ||
| ocdev link nodejs |
There was a problem hiding this comment.
A better example would be to do ocdev link mariadb as you will be injecting variables about mariadb to current component
cmd/link.go
Outdated
| ocdev link nodejs | ||
|
|
||
| # Link 'nodejs' component to 'mariadb' component | ||
| ocdev link nodejs --component mariadb |
There was a problem hiding this comment.
again this should be the other way around ocdev link mariadb --component nodejs
cmd/util.go
Outdated
| checkError(err, "Could not get current component") | ||
| return c | ||
| } | ||
| return inputComponent |
There was a problem hiding this comment.
It would be good to check if the component exists here, so we don't have to do it each time we call this function.
(we need to check it only if len(inputComponent)!=0
pkg/component/component.go
Outdated
| @@ -7,11 +7,15 @@ import ( | |||
| buildv1 "github.com/openshift/api/build/v1" | |||
| "github.com/pkg/errors" | |||
| log "github.com/sirupsen/logrus" | |||
| corev1 "k8s.io/api/core/v1" | |||
|
|
|||
| "strings" | |||
There was a problem hiding this comment.
please group this with other standard library imports
pkg/component/component.go
Outdated
| // component as environment variables | ||
| func Link(client *occlient.Client, sourceComponent, targetComponent, applicationName string) (*LinkInfo, error) { | ||
| // Get Service associated with the target component | ||
| targetService, err := client.GetOneServiceFromSelector( |
There was a problem hiding this comment.
my head hurts from trying to parse this :-(
Would it be better to have separate this? It is hard to parse 3 nested function calls :(
| } | ||
|
|
||
| // getPortFromService returns the first port listed in the service | ||
| func getPortFromService(service *corev1.Service) (int32, error) { |
There was a problem hiding this comment.
Do we really need a separate function that is just one if statement and nothing more? This is used just in one place.
There was a problem hiding this comment.
imo it makes the Link() function look cleaner
There was a problem hiding this comment.
IMO it makes it harder to read ;-)
| checkError(err, "Could not get current component") | ||
| return c | ||
| } | ||
| exists, err := component.Exists(client, inputComponent, applicationName, projectName) |
There was a problem hiding this comment.
Do we need to check that component exists if we got it from GetCurrent?
Exists is expensive it might be enough to put into else block for previous if.
There was a problem hiding this comment.
we're not checking if we're getting it from GetCurrent. The if block has a return statement, we don't need an else here, wdyt
There was a problem hiding this comment.
ah, sorry, Didn't notice return there. LGTM than ;-)
concaf
removed
the
needs-rebase
This commit adds the `ocdev link` command which injects the connection information from the target component to the source component
This commit adds the
ocdev linkcommand which injects theconnection information from the target component to the source
component