Skip to main content

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

  • From: Edward Pilatowicz < >
  • To: Erik Trauschke < >
  • Cc: ,
  • Subject: [pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant
  • Date: Mon, 12 Aug 2013 12:19:10 -0700

On Mon, Aug 12, 2013 at 11:59:49AM -0700, Erik Trauschke wrote:
>
>
> On 08/12/13 11:41 AM, Edward Pilatowicz wrote:
> >On Mon, Aug 12, 2013 at 10:49:38AM -0700, Yiteng Zhang wrote:
> >>On 08/11/13 01:40 PM, Shawn Walker wrote:
> >>>On 08/09/13 18:54, Yiteng Zhang wrote:
> >>>>On 08/ 9/13 02:32 PM, Shawn Walker wrote:
> >>>>>On 08/09/13 12:54, Yiteng Zhang wrote:
> >>>>>>Hi guys,
> >>>>>>     I just fixed thsi bug and push my code review to the
> >>>>>>webrev. Please
> >>>>>>take a look on it.
> >>>>>>     http://ips.java.net/webrev/yitezhan/16986557_prefix_1/
> >>>>>>
> >>>>>>     One thing to mention, after I run the test scripts, it reported
> >>>>>>some mismatches failures. It is because the test scripts
> >>>>>>still expect to
> >>>>>>see the output with prefixes. Thanks.
> >>>>>
> >>>>>
> >>>>>src/client.py:
> >>>>>  Unfortunately, this set of changes will drop the 'variant.' and
> >>>>>'facet.' prefixes from more than just the default output.  The
> >>>>>original bug requests that the prefixes are only omitted from
> >>>>>the default output.
> >>>>>
> >>>>>In other words, you should only omit the prefixes if the
> >>>>>output_format != "default".
> >>>>>
> >>>>>Also, as a matter of style, we generally do not use '' to delimit
> >>>>>strings or even single characters; we use "" instead.  (As seen
> >>>>>on lines 4800, 4801, 4811, 4812, 4890, 4891.)
> >>>>>
> >>>>>
> >>>>>You should be able to fix all of the remaining tests by simply
> >>>>>changing __assert_facet_matches and __assert_variant_matches in
> >>>>>src/tests/cli/t_pkg_varcet.py to remove the 'facet.' or 'variant.'
> >>>>>prefix from the exp_def string variable.
> >>>>>
> >>>>>-Shawn
> >>>>Hi,
> >>>>     Sorry about that I misunderstood the meaning of default output.
> >>>>Thanks for your advise. Now I only omit the prefixed if the
> >>>>output_format == "default" (I think you mean equal). Also, I modified
> >>>
> >>>Yes, I meant output_format == "default"; sorry about that.
> >>>
> >>>>the t_pkg_varcet.py in order to pass the test cases. My code review can
> >>>>be seen at:
> >>>>     http://ips.java.net/webrev/yitezhan/16986557_prefix_2/
> >>>
> >>>src/client.py:
> >>>  This is now correct.
> >>>
> >>>src/tests/cli/t_pkg_varcet.py
> >>>  line 119: We always place a space before and after operators such as
> >>>     '-', '+', etc.  However, read on.
> >>>
> >>>  lines 118-121:  This could be more simply written as:
> >>>
> >>>    expected = "".join(
> >>>        (prefix + l)
> >>>        for l in expected.splitlines(True)
> >>>    )
> >>>
> >>>After you make these changes, be certain you perform a full test
> >>>suite run to ensure nothing else has broken.
> >>>
> >>>You can run the test suite faster by using the -j option to have
> >>>it run tests in parallel where possible, like this:
> >>>
> >>>  python tests/run.py -j8
> >>>
> >>>Assuming the test suite passes, you can push your changeset.
> >>>
> >>>-Shawn
> >>Hi Shawn,
> >>     Thanks, I have made these changes and a full test suite was
> >>passed. I wish I can push the code, but I am not authorized to write
> >>to the repository ips~pkg-gate.
> >>
> >>-Yiteng
> >
> >hey yiteng,
> >
> >i think this change will break kernel zone installs on machines that
> >have facet version locks relaxed.  take a look in kz_install.ksh and
> >you'll see that we iterate over "pkg facet" output looking for facets
> >that begin with "facet.". i recently modified this code and
> >unfortunately i didn't switch it over to using the parsable output
> >(which would have solved this problem for you as well).
>
> Was this part of bug 16948900?
> I checked that the fix was actually put back before I gave the bug
> to Yiteng. If it's not part of the bug above, is there another one
> open for this or are we still unsure how we wanna implement this?
>

no.  that wasn't part of 16948900.  (or 17201126, the other fix i
recently did that i was alluding to above).

i just filed:
    17301112 kernel zone install should use pkg facet parsable output

i think the right way to fix this issue to to change kz_install.ksh so
it uses '-F tsv' parsable output from the pkg facet command since that
output is not changing, a pretty trivial change.

ed


[pkg-discuss] Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/09/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Danek Duvall 08/09/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Shawn Walker 08/09/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/10/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Shawn Walker 08/11/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Erik Trauschke 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Shawn Walker 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Shawn Walker 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/15/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/15/2013
 
 
Close
loading
Please Confirm
Close