Skip to main content

[pkg-discuss] Re: review request: linked image facet inheritance

  • From: Edward Pilatowicz < >
  • To: Shawn Walker < >
  • Cc:
  • Subject: [pkg-discuss] Re: review request: linked image facet inheritance
  • Date: Wed, 7 Aug 2013 12:14:29 -0700

replies are inline below.

a new webrev is available at:
        https://ips.java.net/webrev/edp/15743323.4/

a diff against the previous webrev (57 loc) is at:
        https://ips.java.net/webrev/edp/15743323.4.diff/

thanks again,
ed

On Tue, Aug 06, 2013 at 05:00:17PM -0700, Shawn Walker wrote:
> On 08/06/13 09:33, Edward Pilatowicz wrote:
> >replies are inline below.
> >
> >a new webrev is available at:
> >     https://ips.java.net/webrev/edp/15743323.3/
> >
> >a diff against the previous webrev (145 loc) is at:
> >     https://ips.java.net/webrev/edp/15743323.3.diff/
>
> My replies are against the diff.
>
> Be sure to file a separate bug just for the man page changes for Alta.
>
> >ed
> >
> >On Sun, Aug 04, 2013 at 07:26:41PM -0700, Shawn Walker wrote:
> >>On 07/26/13 18:28, Edward Pilatowicz wrote:
> ...
> >>src/client.py:
> >>   line 4745: I see you accounted for list_masked here, but
> >>     we don't actually have any command line options for it
> >>     or do anything with it.  Any need to keep this?
> >>
> >
> >list_facet() and list_variant() both use the common options processing
> >and both use the same options table definition, client.py`opts_list_varcet.
> >so i couldn't add list_masked to just list_facet() without creating
> >separate opts_list_facet() and opts_list_variant() options tables.
>
> But 'opts_list_varcet' is just a list.  It seems like you could
> easily just do this and then drop line 5846 and the list_masked from
> list_variant():
>
> opts_list_facet = \
>     [opts_cb_varcet] + \
>     opts_list_varcet + \
>     [
>     ("list_masked",  False),
> ]
>
> Right?
>

doh.  i missed the fact that there's already separate opts_list_facet
and opts_list_variant arrays.  i thought there was one opts_list_varcet
array and it was shared for both subcommands.  i've updated this so that
only opts_list_facet has the new option.

> >>src/modules/client/api.py:
> >>   line 894: A *brief* comment explaining why it's ok to ignore wildcard
> >>     matches and fallback to _match_src might be helpful.
> >>
> >>
> >
> >i've added the following comment:
> >
> >                                 # this facet is not set (directly or 
> > indirectly
> >                                 # via wildcards) so the system will 
> > assign it
> >                                 # a default value.
>
> Here's why that's confusing to me -- the condition that's being
> checked for is "if name not in facets"; but doesn't that only return
> True for exact entries and not wildcard matches?
>

oops.  i screwed that up.  i changed the code to read:

                        # check if the facet is explicitly set.
                        if name not in facets:
                                # The image's Facets dictionary will return
                                # the effective value for any facets not
                                # explicitly set in the image (wildcards or
                                # implicit). _match_src() will tell us how
                                # that effective value was determined (via a
                                # local or inherited wildcard facet, or via a
                                # system default).
                                ...

                        # This is an explicitly set facet.

does this make things clearer?

> >>   lines 3219-3220: Using the 'known' catalog from the image here
> >>     should be faster and use less memory than using the manifest.
> >
> >i don't understand what you mean.  are your suggesting using different
> >interfaces to iterate over the manifests and actions in place of
> >get_manifest() and gen_actions*()?
>
> I was suggesting using the catalog interfaces instead of the
> Manifest interfaces; for an example, see lines 4521-4528 in
> image.py.
>
> But this is really just potential optimisation work, so you could
> file an ER for that to avoid delaying pushing your changes.
>

ah.  ok.  i switched to using the catalog interfaces for iterating over
dependency actions.  this allowed me to delete all my changes to
manifest.py.

i have clean baselines and one more set of manual tests i want to run,
so assuming this looks ok i think i can probably still get this
integrated today.


[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/05/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/06/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/06/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/07/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/07/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/08/2013
 
 
Close
loading
Please Confirm
Close