On 02/29/12 09:58, Dave Miner wrote:
On 02/28/12 19:10, Ethan Quach wrote:
On 02/17/12 08:11, Dave Miner wrote:
On 02/16/12 16:39, Ethan Quach wrote:
On 02/07/12 08:14, Tomas Dzik wrote:
Hi Ethan,
thanks a lot for your review and comments. I fixed most of them. For
the rest please see in-line.
New webrev is here:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7120099-1
Please let me know whether it is acceptable now.
Dne 3.02.12 01:49, Ethan Quach napsal(a):
Tomas,
Thanks for fixing this. A few comments ...
282-297 - Couldn't you just call AIdb.getTableCriteria() with
humanOutput=True at line 282 to prevent having to call it again at
292?
I intentionally call it twice here because its hard to distinguish
empty criteria in human readable output.
Why do we need to distinguish empty criteria if the calls are being
called with onlyUsed=True; doesn't that only return the criteria that
are non-empty?
889 - Is it possible to make this class support both -- printing
manifests for all services, or for just one? To me, it seems
awkward for
the code at 1487,1491 to use two different classes to print one
case or
the other. Why can't this class just take an optional service_name
argument for cases where only one service needs to be printed?
I would prefer not to do it because both options substantially differ
- they use different header and different output format. Also
there is
different output format if there is just one manifest/profile
printed by
installadm list -p
root@S11:~# installadm list -p
Service/Profile Name Criteria
-------------------- --------
solaris11u1-i386-06
some-new-profile None
and if the same manifest is printed using installadm list -p -n
root@S11:~# installadm list -p -n solaris11u1-i386-06
Profile Criteria
------- --------
some-new-profile None
Bikeshedding a bit, I would like to suggest we use a lot less
whitespace in the output below. The extra spacing for each profile
seems quite unnecessary, as does the extra spacing between each
service and its profiles.
The current whitespace between each profile is so that things look more
clear for cases where a profile has multiple criteria:
One way we might address that is to swap the Criteria and Status
columns, seems like it might be more scannable as:
Service/Manifest Name Criteria Status
--------------------- -------- ------
solaris11-i386
HOST mac = 00:11:22:33:44:55
ipv4 = 10.0.0.1
cpu = i386
aiC-2 mac = 08:00:27:CC:25:B9
ipv4 = 10.0.0.2
cpu = i386
bar arch = foobar
orig_default None Default
foo None Inactive
This makes it slightly better, but to me still a bit jumbled.
What if we detect when a manifest has multiple criteria, and dynamically
insert white space only around those?
Service/Manifest Name Status Criteria
--------------------- ------ --------
solaris11-i386
HOST mac = 00:11:22:33:44:55
ipv4 = 10.0.0.1
cpu = i386
aiC-2 mac = 08:00:27:CC:25:B9
ipv4 = 10.0.0.2
cpu = i386
bar arch = foobar
manifest3 mac = aa:bb:cc:dd:ee:ff
orig_default Default None
foo Inactive None
Or, more radically, we collapse Status and Criteria:
Service/Manifest Name Criteria
--------------------- --------
solaris11-i386
HOST mac = 00:11:22:33:44:55
ipv4 = 10.0.0.1
cpu = i386
aiC-2 mac = 08:00:27:CC:25:B9
ipv4 = 10.0.0.2
cpu = i386
bar arch = foobar
orig_default Default
foo Inactive
If you made HOST the default:
Service/Manifest Name Criteria
--------------------- --------
solaris11-i386
HOST Default
(mac = 00:11:22:33:44:55)
(ipv4 = 10.0.0.1)
(cpu = i386)
aiC-2 mac = 08:00:27:CC:25:B9
ipv4 = 10.0.0.2
cpu = i386
bar arch = foobar
orig_default Inactive
foo Inactive
Thoughts?
Not sure I like collapsing them. With the column name being 'Criteria',
its not clear if Inactive means the manifest is inactive, or if just the
criteria associated with that manifest are inactive, but somehow the
manifest is still usable ...
-ethan
Dave
[equach@ethos]:/> installadm list -m -n solaris11-i386
Service/Manifest Name Status Criteria
--------------------- ------ --------
solaris11-i386
HOST mac = 00:11:22:33:44:55
ipv4 = 10.0.0.1
cpu = i386
aiC-2 mac = 08:00:27:CC:25:B9
ipv4 = 10.0.0.2
cpu = i386
bar arch = foobar
orig_default Default None
foo Inactive None
VS.
[equach@ethos]:/> installadm list -m -n solaris11-i386
Service/Manifest Name Status Criteria
--------------------- ------ --------
solaris11-i386
HOST mac = 00:11:22:33:44:55
ipv4 = 10.0.0.1
cpu = i386
aiC-2 mac = 08:00:27:CC:25:B9
ipv4 = 10.0.0.2
cpu = i386
bar arch = foobar
orig_default Default None
foo Inactive None
I could live with this though. For the cases where there are adjacent
profiles the have only one criteria, which is the more common case, its
does make that look a tad bit more readable.
I don't believe there is a whitespace between the service name and its
profiles. That somehow came out in the cut-n-paste job that I did into
the last email as preformat text.
-ethan
But maybe it's just me.
Dave
Understood. I would be ok with changing that to make the header and
output always be consistent with the first form of output above. For
example,
root@S11:~# installadm list -p
Service/Profile Name Criteria
-------------------- --------
solaris11u1-i386-06
some-new-profile None
service2
profile-foo None
profile-bar None
profile-blah None
service3
profile-X None
root@S11:~# installadm list -p -n service2
Service/Profile Name Criteria
-------------------- --------
service2
profile-foo None
profile-bar None
profile-blah None
Also ServicesManifestList() class contains list of
ServiceManifestPrint() classes and prints them by iterating through
them. So basically ServicesManifestList() is container for
ServiceManifestPrint().
However if you insist on changing it I can do it, but it would not be
nice and the switch between these 2 options would just be moved into
class itself which is not nice either. Just let me know.
Yes, I would prefer that. Given the above, I think doing this makes
more
sense.
One new question I have is regarding how lines get printed. The
objects
seem to all return strings, and the entirety of everything that
will get
printed is accumulated into a large string buffer. Are there any
limitations with this if the output is very large?
Also, I just tried your new list.py; there now seems to be an
additional
empty line of output right after the command is invoked. This was not
there before, nor was it there in the first webrev.
thanks,
-ethan
Best regards,
Tomas D.
thanks,
-ethan
On 01/20/12 04:10, Tomas Dzik wrote:
Hi all,
I would like to kindly ask whether someone has some spare time to
review my changes.
I know that these changes are quite substantial.
Best regards,
Tomas D.
Dne 30.12.11 15:46, Tomas Dzik napsal(a):
Hi all,
I would like to ask you for a code review for:
7120099 - "installadm list -p" should show criteria
This fix contains also changes to the way how "installadm list -m"
works.
Webrev:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7120099/
Testing:
1) Sources are pep8 clean
2) Test suite in the gate pass
3) I created several services with different manifests (Default,
Active,
Inactive) with several sets of criteria and verified that these
are
printed as before
4) I also created profiles with different criteria and verified
how
they
are printed.
5) I tried to change the version of installed service and verified
that
code correctly handles this exception.
I changed a little bit format how ignored criteria are printed (as
suggested in one of the previous code reviews).
Instead:
# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default (Ignored: ipv4 = 192.168.1.2)
(Ignored: mem = 2048 MB - unbounded)
I now print:
# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default (Ignored:
ipv4 = 192.168.1.2
mem = 2048 MB - unbounded
)
Best regards,
Tomas D.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
