-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding machine output to component create #2532
Adding machine output to component create #2532
Conversation
f6d94b2
to
5eae7db
Compare
pkg/odo/cli/component/push.go
Outdated
return err | ||
} | ||
if log.IsJSON() { | ||
componentDesc, err := component.GetComponent(po.Context.Client, po.Context.Component(), po.Context.Application, po.Context.Project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if outputting component description is what we want for odo push -o json
output.
I would skip JSON output for the push for now as we are not sure how it should look like. This PR was supposed to just about create command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, skipped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output was supposed to be for odo create
command, not odo push
:-(
▶ odo component create java -o json
✗ Machine readable output is not yet implemented for this command
5eae7db
to
42d92b7
Compare
@kadel fixed |
@mohammedzee1000 :-(
|
any update on this PR @mohammedzee1000 |
I am looking at what broke |
please address @kadel comments too |
64ca790
to
e773574
Compare
Codecov Report
@@ Coverage Diff @@
## master #2532 +/- ##
=======================================
Coverage 43.39% 43.39%
=======================================
Files 91 91
Lines 8312 8312
=======================================
Hits 3607 3607
Misses 4352 4352
Partials 353 353 Continue to review full report at Codecov.
|
Due to less amount of time, I am removing the changes i had implemented to json output itself. I am recording it #2573. I will push smaller PR with only machine-readable output for component creation shortly. |
fc3cc1a
to
a660515
Compare
1517fec
to
3cadb5a
Compare
Looks like only this is currently implemented: ~/nodejs-ex master ✔ 16d ⍉
▶ odo create nodejs -o json
{
"metadata": {
"creationTimestamp": null
},
"spec": {},
"status": {
"state": ""
}
} However.. this should match {
"kind": "Component",
"apiVersion": "odo.openshift.io/v1alpha1",
"metadata": {
"name": "nodejs-nodejs-ex-wila",
"namespace": "default",
"creationTimestamp": null
},
"spec": {
"app": "app",
"type": "nodejs",
"source": "file://./",
"ports": [
"8080/TCP"
]
},
"status": {
"state": "Not Pushed"
}
} So I'm going to fix that 👍 |
983b696
to
15fa4c1
Compare
0b937be
to
43a7275
Compare
@mohammedzee1000 How is this progressing? Any blockers? |
tests/integration/component.go
Outdated
@@ -236,6 +236,24 @@ func componentTests(args ...string) { | |||
helper.CmdShouldPass("odo", append(args, "delete", "-f", "--all", "--context", context)...) | |||
}) | |||
|
|||
It("should describe not pushed component when it is created with json output", func() { | |||
cmpDescribeJSON, err := helper.Unindented(helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--context", context, "--app", "testing", "-o", "json")...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use nodejs local source (less time consuming) instead of git source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tests/integration/component.go
Outdated
}) | ||
|
||
It("should describe pushed component when it is created with json output", func() { | ||
cmpDescribeJSON, err := helper.Unindented(helper.CmdShouldPass("odo", append(args, "create", "nodejs", "cmp-git", "--project", project, "--git", "https://github.com/openshift/nodejs-ex", "--context", context, "--app", "testing", "-o", "json", "--now")...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use local nodejs source to save few minutes of CI time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>
7f664da
to
3b9e7c7
Compare
@amitkrout done |
/retest |
1 similar comment
/retest |
/lgtm |
return err | ||
} | ||
} | ||
componentDesc.Spec.Ports = co.LocalConfigInfo.GetPorts() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed to be done, shouldn’t the port be populated in the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took what is there in odo describe
logic. Must be a reason why its there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great!
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdrage The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
Which issue(s) this PR fixes:
Fixes #2309
How to test changes / Special notes to the reviewer: