Include additional parts of deploy manifest step #4
Conversation
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
| conditions := status["conditions"].([]interface{}) | ||
| for i := range conditions { | ||
| c := conditions[i].(map[string]interface{}) | ||
| klog.V(4).Infof("Condition returned\n%s\n", c) |
There was a problem hiding this comment.
should you put this log inside the if?
There was a problem hiding this comment.
I wanted to be able to see what all the conditions we had were, so had it outside. Will probably remove later.
| deployJSON, err := deploymentManifest.MarshalJSON() | ||
| if err != nil { | ||
| return errors.Wrap(err, "Deployment manifest is invalid") | ||
| } | ||
| klog.V(3).Infof("Deploy manifest:\n\n%s\n", deployJSON) |
There was a problem hiding this comment.
Why do we need it in JSON? It is just to print it out?
There was a problem hiding this comment.
I was just using that to test that the manifest was valid, and then thought I could print it out.
| // Need to wait for a second to give the server time to create the artifacts | ||
| time.Sleep(2 * time.Second) | ||
|
|
There was a problem hiding this comment.
Just needs a TODO to replace with wait
|
|
||
| // Check to see whether deployed resource already exists. If not, create else update | ||
| instanceFound := false | ||
| list, err := a.Client.DynamicClient.Resource(gvr).Namespace(namespace).List(metav1.ListOptions{}) |
There was a problem hiding this comment.
could we do a Get() instead of List()?
There was a problem hiding this comment.
Yes, probably. Will do that in a new PR though!
There was a problem hiding this comment.
Have made change to use Get rather than List.
| deploymentManifest.SetUnstructuredContent(newService) | ||
| } | ||
| } | ||
| instanceFound = true |
There was a problem hiding this comment.
should we break from the for here?
There was a problem hiding this comment.
Yes, but if we do a Get rather than a List it wont be an issue and this code will get refactored. Will fix in new PR.
There was a problem hiding this comment.
Use of Get had made doing a break unecessary.
| log.Infof("Updating %s...", gvk.Kind) | ||
| result, err = myclient.Resource(gvr).Namespace(namespace).Update(deploymentManifest, metav1.UpdateOptions{}) | ||
| } | ||
| _ = manifestFile.Close() |
There was a problem hiding this comment.
we would want to return this error. I would rather write a defer where we create the files as such:
defer func() {
merr := manifestFile.Close()
if err == nil {
err = merr
}
}()
Signed-off-by: Steven Groeger <groeges@uk.ibm.com>
|
@EnriqueL8 Have made a few changes as requested and pushed again to the branch. Hopefully this will be OK to merge. |
| @@ -62,7 +62,7 @@ func (po *PushOptions) DevfilePush() (err error) { | |||
| platformContext = nil | |||
| } else { | |||
| kc := kubernetes.KubernetesContext{ | |||
| Namespace: po.namespace, | |||
| Namespace: po.KClient.Namespace, | |||
What type of PR is this?
/kind feature
What does does this PR do / why we need it:
Which issue(s) this PR fixes:
Fixes #?
How to test changes / Special notes to the reviewer: