Make --output a global parameter, disable logging, fix bug #1898
Conversation
|
/hold making some changes for debugging |
79140f1 to
4a729cf
Compare
|
/retest |
a3212be to
140c9fd
Compare
|
Had to combine some of my commits, but this is ready for review 👍 @surajnarwade @kadel @mik-dass @amitkrout |
18fe8ef to
b3355cb
Compare
|
#1782 (comment) |
/retest |
pkg/log/status.go
Outdated
| // set new status | ||
| isTerm := IsTerminal(s.writer) | ||
| s.status = status | ||
|
|
||
| // If we are in debug mode, don't spin! | ||
| if !isTerm || debug { | ||
| fmt.Fprintf(s.writer, prefixSpacing+getSpacingString()+suffixSpacing+"%s ...\n", s.status) | ||
| } else { | ||
| } else if !json { |
There was a problem hiding this comment.
why not use IsJSON() function here too instead of passing a extra parameter?
There was a problem hiding this comment.
You're right, I should of changed the old code here.. Thanks!
|
@mik-dass thanks for the review, updated again! |
|
/retest |
pkg/odo/cli/cli.go
Outdated
| // We use "flag" in order to make this accessible throughtout ALL of odo, rather than the | ||
| // above traditional "persistentflags" usage that does not make it a pointer within the 'pflag' | ||
| // package | ||
| flag.CommandLine.String("o", "", "Specify output format, supported format: json") |
There was a problem hiding this comment.
/retest |
19b134e to
c030b08
Compare
|
/retest |
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mik-dass 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 |
ee5b504 to
f8f5e19
Compare
|
Tests finally pass 👍 |
This PR: - Makes `-o` or `--output` a global parameter - Disables logging completely when performing `-o json`, including using verbosity `-v` - Fixes a minor bug within some commands where `-o json` wasn't detected corretly due to a local flag being used `outputFlag` rather than `OutputFlag`
f8f5e19 to
fb0fd8b
Compare
|
/retest |
There was a problem hiding this comment.
Codewise looking good. Not sure why we have failure related to looking for a value FOO.
https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/1898/pull-ci-openshift-odo-master-integration/1054#0:build-log.txt%3A8438
At initial glance, does not seem to be related to this code cgange
The test in question was added by me as a part of #1860. I believe I should have modified the code to check for If this fails repeatedly at the same place, we can try doing that change. |
|
/test integration |
@dharmit The issue is hitting due to line https://github.com/openshift/odo/blob/master/tests/integration/component.go#L507. Due to the parallel run on two test node will fix the issue |
There was a problem hiding this comment.
This PR:
Makes -o or --output a global parameter
@cdrage Thanks for the pr. Can you please add some more context of why we need this change.
Disables logging completely when performing -o json, including
using verbosity -v
+1, make sense
Fixes a minor bug within some commands where -o json wasn't
detected corretly due to a local flag being used outputFlag rather
than OutputFlag
Looks like a bug fix. Can you guide the steps to reproduce it. I am asking because it will be helpful to analyze/write the possible test case if any.
|
/hold cancel |
|
/refresh |
This PR:
-oor--outputa global parameter-o json, includingusing verbosity
-v-o jsonwasn'tdetected corretly due to a local flag being used
outputFlagratherthan
OutputFlag