common: adding line break at end of some cli results #16687

Songweibin · 2017-07-31T08:41:12Z

I get rbd list info as follows, when using --format xml.

[root@s248 ~]# rbd ls --format xml
<images><name>i1</name></images>[root@s248 ~]# 
[root@s248 ~]#

This occurs when only --format used, and after fixup as follows:

[root@s248 build]# ./bin/rbd ls --format xml
<images><name>i1</name></images>
[root@s248 build]#

If --pretty-format used while --format not works, it does not return error:

[root@s248 ~]# rbd ls --pretty-format
i1
[root@s248 ~]#

fixes: http://tracker.ceph.com/issues/21019
Signed-off-by: songweibin song.weibin@zte.com.cn

Songweibin · 2017-07-31T12:30:52Z

Jenkins retest this please

dillaman · 2017-07-31T13:50:55Z

This should be in the CLI and not in the low-level formatter.

Songweibin · 2017-08-01T12:10:10Z

@dillaman Modified and Updated. Thanks for your review. I'm wondering if this is that you mean?

dillaman

It might be nicer to add a setter to the JSON and XML formatter to optionally add a newline at the end. That way, only Utils.cc needs to be updated to enable that option.

dillaman · 2017-08-01T13:00:56Z

doc/man/8/rbd.rst

@@ -120,7 +120,8 @@ Parameters

 .. option:: --pretty-format

-   Make json or xml formatted output more human-readable.
+   Make json or xml formatted output more human-readable, and it only works


Nit: seems redundant to me since it already states it's for making json or xml output.

dillaman · 2017-08-01T13:03:35Z

src/tools/rbd/ArgumentTypes.cc

@@ -332,7 +332,7 @@ void add_no_progress_option(boost::program_options::options_description *opt) {

 void add_format_options(boost::program_options::options_description *opt) {
  opt->add_options()
-    (FORMAT.c_str(), po::value<Format>(), "output format [plain, json, or xml]")
+    (FORMAT.c_str(), po::value<Format>(), "output format [default: plain, json, or xml]")


Nit: probably should make it "output format (plain, json, or xml) [default: plain]"

dillaman · 2017-08-01T13:04:12Z

src/tools/rbd/Utils.cc

@@ -832,6 +832,10 @@ int get_formatter(const po::variables_map &vm,
                << "is json or xml" << std::endl;
      return -EINVAL;
    }
+  } else if (vm[at::PRETTY_FORMAT].as<bool>()) {


Nit: should be vm[at::PRETTY_FORMAT].count() > 0

Nit: doesn't stop a user from running "--format plain --pretty-format"

Seems vm[at::PRETTY_FORMAT].count() (or maybe I missed something in it) is wrong when compiling and vm.count(at::PRETTY_FORMAT) always equal to 1, I used vm[at::PRETTY_FORMAT].as<bool>() > 0.
Others have been updated, thanks for your review.

Ah -- it probably has a default specified

Songweibin · 2017-08-02T06:28:40Z

Jenkins retest this please

dillaman · 2017-08-02T16:23:09Z

src/tools/rbd/Utils.cc

  }
  return 0;
 }

+int add_newline_at_end(const po::variables_map &vm,


I was suggesting adding a boolean setter to the Formatter that would be enabled by the rbd CLI if using json / xml non-pretty formatter.

Signed-off-by: songweibin <song.weibin@zte.com.cn>

Songweibin · 2017-08-09T07:22:06Z

@dillaman Oops, I misunderstood your suggestion. Already modified and repushed, thank you.

Songweibin · 2017-08-09T07:22:39Z

Jenkins retest this please

dillaman · 2017-08-09T13:55:22Z

src/common/Formatter.cc

@@ -117,8 +117,10 @@ void Formatter::dump_format_unquoted(const char *name, const char *fmt, ...)

 // -----------------------

-JSONFormatter::JSONFormatter(bool p)
-: m_pretty(p), m_is_pending_string(false)
+JSONFormatter::JSONFormatter(bool pretty, bool format)


Nit: can you just add a setter method (i.e. enable_line_break()) -- I'd like to avoid creating new defaulted parameters.

dillaman · 2017-08-09T13:55:54Z

src/common/Formatter.h

@@ -128,12 +128,14 @@ namespace ceph {
    std::stringstream m_ss, m_pending_string;
    std::list<json_formatter_stack_entry_d> m_stack;
    bool m_is_pending_string;
+    bool m_format;


Nit: m_format is pretty generic sounding -- perhaps something like m_line_break_enabled or similar.

dillaman · 2017-08-09T13:56:50Z

src/tools/rbd/ArgumentTypes.h

@@ -111,7 +111,7 @@ struct Format : public TypedValue<std::string> {

  Format(const std::string &format) : TypedValue<std::string>(format) {}

-  Formatter create_formatter(bool pretty) const;
+  Formatter create_formatter(bool pretty, bool format) const;


Nit: seems like it's always going to be true so no need for a new param

ceph-jenkins · 2017-08-14T04:29:15Z

all commits in this PR are signed

Songweibin · 2017-08-14T05:24:49Z

@dillaman Repushed, thanks for your review and suggestion.

dillaman · 2017-08-16T12:54:07Z

src/common/Formatter.cc

@@ -326,6 +328,8 @@ void XMLFormatter::flush(std::ostream& os)
   * we should NOT output a newline. This primarily triggers on HTTP redirects */
  if (m_pretty && !m_ss_str.empty())
    os << "\n";
+  if (m_line_break_enabled)


Nit: else if to avoid possibility of a double line break if in the future someone enables it and doesn't guard against pretty printing also being enabled

Songweibin · 2017-08-16T15:45:15Z

@dillaman Yeah, thanks, done.

Songweibin · 2017-08-17T01:19:02Z

Jenkins retest this please

Songweibin · 2017-08-17T05:36:29Z

Jenkins retest this please

Songweibin · 2017-08-17T10:03:30Z

Jenkins retest this please

dillaman

lgtm

dillaman · 2017-08-17T19:49:55Z

cram test needs to be updated: http://qa-proxy.ceph.com/teuthology/jdillaman-2017-08-17_14:06:40-rbd-wip-jd-testing-distro-basic-smithi/1536040/teuthology.log

Songweibin · 2017-08-18T00:53:03Z

@dillaman Updated, thank you.

Signed-off-by: songweibin <song.weibin@zte.com.cn>

Songweibin · 2017-08-18T04:54:16Z

tests contain formatted-output.yaml: http://pulpito.ceph.com/xxg-2017-08-18_04:25:36-rbd-wip-xxg-working-distro-basic-smithi/

Songweibin · 2017-08-18T05:58:21Z

Jenkins retest this please

smithfarm · 2017-08-18T10:09:53Z

Presumably we'd want this for luminous as well? In that case, @Songweibin can you add

Fixes: http://tracker.ceph.com/issues/21019

just above the Signed-off-by line? Thanks.

dillaman · 2017-08-18T11:03:51Z

@smithfarm that tracker ticket looks unrelated

smithfarm · 2017-08-18T11:16:00Z

@dillaman Does 19dd420 not fix http://tracker.ceph.com/issues/21019 ? If not, then perhaps @Songweibin could open a new tracker ticket for this PR and write "luminous" in the backport field?

dillaman · 2017-08-18T12:25:29Z

@smithfarm ah -- 19dd420 was added after my last review.

Songweibin · 2017-08-18T13:46:01Z

@dillaman @smithfarm Sorry for forget to add tracker ticket. Done, thank you so much.

dillaman · 2017-08-18T13:48:26Z

re-adding the needs-qa label since the addition of 19dd420 could affect multiple test suites outside of rbd

tchaikov · 2017-08-20T05:58:59Z

joscollin added cleanup common rbd labels Jul 31, 2017

Songweibin mentioned this pull request Jul 31, 2017

common: CLI result should have a line break when only --format works #16441

Closed

Songweibin force-pushed the wip-xml-format branch 2 times, most recently from cda83be to ec92b59 Compare August 1, 2017 11:47

Songweibin changed the title ~~common: adding newline operation when using '--format xml'~~ rbd: adding newline operation when only '--format' used and return error when only '--pretty-format' used Aug 1, 2017

dillaman reviewed Aug 1, 2017

View reviewed changes

Songweibin force-pushed the wip-xml-format branch from ec92b59 to c091ed4 Compare August 2, 2017 05:30

dillaman reviewed Aug 2, 2017

View reviewed changes

rbd: '--pretty-format' should return error if '--format' not used

47e7549

Signed-off-by: songweibin <song.weibin@zte.com.cn>

Songweibin force-pushed the wip-xml-format branch from c091ed4 to 3ee6ea8 Compare August 9, 2017 06:25

dillaman reviewed Aug 9, 2017

View reviewed changes

Songweibin force-pushed the wip-xml-format branch from 3ee6ea8 to e48b641 Compare August 14, 2017 04:28

dillaman reviewed Aug 16, 2017

View reviewed changes

Songweibin force-pushed the wip-xml-format branch from e48b641 to 87e76e4 Compare August 16, 2017 15:38

Songweibin mentioned this pull request Aug 16, 2017

[DNM]common: add new line break when get command descriptions #17051

Closed

Songweibin changed the title ~~rbd: adding newline operation when only '--format' used and return error when only '--pretty-format' used~~ misc: adding line break at end of some cli results Aug 17, 2017

dillaman approved these changes Aug 17, 2017

View reviewed changes

dillaman added the needs-qa label Aug 17, 2017

dillaman changed the title ~~misc: adding line break at end of some cli results~~ rbd: adding line break at end of some cli results Aug 17, 2017

dillaman added wip-jason-testing and removed needs-qa wip-jason-testing labels Aug 17, 2017

Songweibin force-pushed the wip-xml-format branch from 84c20d6 to 19dd420 Compare August 18, 2017 00:50

Songweibin added 2 commits August 18, 2017 08:54

rbd: adding newline operation when only '--format' used

992829d

Signed-off-by: songweibin <song.weibin@zte.com.cn>

common: add new line break when get command descriptions

19dd420

Signed-off-by: songweibin <song.weibin@zte.com.cn>

dillaman added the needs-qa label Aug 18, 2017

dillaman changed the title ~~rbd: adding line break at end of some cli results~~ common: adding line break at end of some cli results Aug 18, 2017

tchaikov added the wip-kefu-testing label Aug 19, 2017

tchaikov merged commit d81816f into ceph:master Aug 20, 2017

Songweibin deleted the wip-xml-format branch September 7, 2017 05:54

common: adding line break at end of some cli results #16687

common: adding line break at end of some cli results #16687

Conversation

Songweibin commented Jul 31, 2017 • edited

Songweibin commented Jul 31, 2017

dillaman commented Jul 31, 2017

Songweibin commented Aug 1, 2017

dillaman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Songweibin commented Aug 2, 2017

Choose a reason for hiding this comment

Songweibin commented Aug 9, 2017

Songweibin commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceph-jenkins commented Aug 14, 2017

Songweibin commented Aug 14, 2017

Choose a reason for hiding this comment

Songweibin commented Aug 16, 2017

Songweibin commented Aug 17, 2017

Songweibin commented Aug 17, 2017

Songweibin commented Aug 17, 2017

dillaman left a comment

Choose a reason for hiding this comment

dillaman commented Aug 17, 2017

Songweibin commented Aug 18, 2017

Songweibin commented Aug 18, 2017

Songweibin commented Aug 18, 2017

smithfarm commented Aug 18, 2017 • edited

dillaman commented Aug 18, 2017

smithfarm commented Aug 18, 2017

dillaman commented Aug 18, 2017

Songweibin commented Aug 18, 2017

dillaman commented Aug 18, 2017

tchaikov commented Aug 20, 2017

Songweibin commented Jul 31, 2017 •

edited

smithfarm commented Aug 18, 2017 •

edited