Skip to main content

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

  • From: Erik Trauschke < >
  • To:
  • Cc: Yiteng Zhang < >
  • Subject: [pkg-discuss] Re: code review request: pkg dehydrate/rehydrate
  • Date: Fri, 06 Jun 2014 10:12:59 -0700



On 05/29/14 04:23 PM, Yiteng Zhang wrote:
Hi folks,

Can I ask you to review on this?

Some notes:
1) pkg dehydrate/rehydrate go through the api planning code path. After
discussion
in the meeting and some experiments, instead of using manifest
comparison to get
the files and hardlinks that are needed to remove in dehydrate or to add in
rehydrate, I generate these actions to a dict and pass it explicitly to
pkg plans to
propose an repair. Details in imageplan.py.

2) Confirmed with Jesse that pkg dehydrate/rehydrate only needs to work on
alternate root.

3) The last change of pkg man pages was pushed to s11.2 only but not to
s12,
which is weird. So there are a lot of change in src/man/pkg.1.

4) Test suite of pkg dehydrate and pkg rehydrate are combined in one
file, client
test in src/tests/cli/t_pkg_hydrate.py, and api test in
src/tests/api/t_pkg_api_hydrate.py.
Test case might not be sufficient, let me know your idea.

5) Manual test on full, partial and user image.
For example, it takes about 44s to dehydrate an user image only
installed with
package core-os. The size of the image goes from 1.2GB to 308MB. And it
takes
about 3m20s to rehydrate the image.

6) Fix "bug 15521086 pkg fix should be moved into the api".

7) Probably I need to file a bug about this project.

webrev: https://ips.java.net/webrev/yitezhan/pkg_hydrate/

Please let me know your opinion.


src/client.py:
general
I think I would call the commands dehydrate and hydrate since rehydrate sounds like hydrate something which was already hydrated. But I don't feel very strongly about that.

1855, 1857
you are using the global img variable here, which seems sketchy. I'd use _api_inst.img instead.

2186, 2193, 2199
Sort arguments after pargs or api_inst alphabetically, like in the other functions.

src/modules/client/api.py:
        Same here, sort arguments alphabetically, the ones starting with _ 
first.

src/modules/client/api_errors.py:
700
You might wanna put a newline between your error description and the list of invalid publishers.

src/modules/client/image.py:
2394:
I don't understand this comment. If I call save_hydrate_property with dehydrate=False and a list of publishers set I will always reach that point. And why is that important in the first place?

src/modules/client/image.py:
4484++
I don't think we want to print stuff from here. That will probably break any other client than pkg(1). The licenses should be shown in client.py:display_plan(). I'm not even sure why we show licenses in fix in the first place.

src/modules/client/imageplan.py:

1410
Why can't we dehydrate with a missing origin? I think we should have all the info we need on disk. Or is this just some safeguard preventing the users from shooting themselves in the foot since they won't be able to hydrate the image again?
1411
        /rasie/raise/
1430
        "Dehydrate will not work with unconfigured publishers."

1436++
What Bart was saying. Hardcoding certain files into the pkg system seems wrong.

1464++
Same as in image.py. You can't print stuff here. This needs to go into it's own progress tracker, which will probably be more work than putting fix into the api.

src/modules/client/options.py:
Please format the option tables the same way as the other ones, for better readability.

src/tests/cli/t_fix.py
189
        /same/the same/

src/tests/pkg5unittest.py
I know you just moved that from _pkg_revert.py but shouldn't it be enough to just make them wrappers for file_exists() and file_doesnt_exist()? You could even just extend the two to accept a single or multiple paths.

Erik






Thanks,
Yiteng


[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

(continued)

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Danek Duvall 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Danek Duvall 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

Message not available

Message not available

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/25/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/25/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/25/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Danek Duvall 06/27/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Erik Trauschke 06/06/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/06/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Erik Trauschke 06/06/2014
 
 
Close
loading
Please Confirm
Close