Tomas,
On 12/12/11 05:17, Tomas Dzik wrote:
Hi Ethan,
you are right and I considered reorganizing code the way you described.
I didn't do that because I am accustomed to do the minimal changes
when fixing bugs and resist temptation to change the code which isn't
broken just because it would look better. (That's the way how
sustaining works ;-) However if you feel that I should clean that up I
will do that. Just let me know.
I agree when its purely aesthetic, but the purpose of re-structuring
here would be to benefit future maintainability of the code which
in-turn, I would think, be valuable for sustaining. However, this
particular case does not seem to be a simple -- "move block of code into
function; call function from old place and new place" -- so I'm not
going to impose it.
In addition to the one nit I had previously, I just had one more comment
below. Other than those, I'm ok with the changes for this fix.
522 - Can you expand on this comment to denote what each entry in the
list contains? (We're including a dictionary of criteria within each
entry now, correct?)
thanks,
-ethan
Regards,
Tomas D.
Dne 9.12.11 23:21, Ethan Quach napsal(a):
Tomas,
General comments:
get_manifest_or_profile_names() and get_mfest_or_profile_criteria() seem
to return almost the same content now since we're including criteria in
the sdict for the former. Did you consider consolidating them into one
function, and adjusting the code that calls them accordingly? Just
wondering how much effort that would be.
Same general question for print_local_manifests() and
print_service_manifests(); the new code introduced at 988-1041 seems to
be very similar code at 838-863. Could these two methods be commonized?
1014 - nit - "inactive" -> "ignored"
thanks,
-ethan
On 12/09/11 05:37, Tomas Dzik wrote:
Hi Ethan,
no issues. It was just copy-paste a block of code. Next time i will
not run "hg recommit" before code review is finished ;-). Anyway could
you please review the original webrev - it means:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7036455-1/
Thanks a lot,
Tomas D.
Dne 8.12.11 18:10, Ethan Quach napsal(a):
On 12/08/11 08:50, Tomas Dzik wrote:
Hi Ethan,
the current output lists the "active" manifests first, then "Default"
and finally "Inactive" so I took this order. However was quite simple
to reorder them so I now list the Default first, then active and
finally Inactive as you suggest.
Ah I was wrong about this. The current order was correct. Can you
change
that back. Sorry about that.
-ethan
The current webrev is here:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7036455-2/
The output is in attachment.
Best regards,
Tomas D.
Dne 8.12.11 00:16, Ethan Quach napsal(a):
Tomas,
Thanks for taking our suggestions. The newly attached output looks
OK to
me, but there's one thing that looks strange. In the New (e)
output, the
Default manifest for the solaris11-i386 service is listed last; that
shouldn't be the case as I thought that we always sort the
manifests and
list the Default first, then "active", and finally Inactive.
Not sure if this is an existing bug or something new in your code...
thanks,
-ethan
On 12/01/11 05:44, Tomas Dzik wrote:
Hi Ethan and Dave,
I implemented your suggestion and repeated the tests. There will
always be cases when line will be longer then 80 chars because
service
name and manifest name can be arbitrarily long. I was also thinking
about not printing Ignored before each criteria and printing it
only
once. On the other hand I don't think that a lot of people will use
ignored criteria.
My new output is in attachment.
New webrev is here:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7036455-1/
Also I noted that (even in the original code) network is printed
ugly
(without dots).
Should I file separate bug and fix it ? (It seems to be in
different
part of code then I am fixing now.)
Regards,
Tomas D.
Dne 30.11.11 22:58, Dave Miner napsal(a):
Ethan, I think that's an improvement. I think we could also put
some
effort into minimizing the whitespace in the criteria column to
improve
readability. Might also consider whether Status could be
compressed to
just D, I, <blank> (there aren't any other states at this point,
right?).
Dave
On 11/29/11 15:56, Ethan Quach wrote:
Tomas, Dave,
One suggestion is to merge the Service and Manifest name columns
into
one by outputting the service name in it's own line, and using a
static
3-space indentation for each manifest entry that belongs to that
service
beneath it. This removes the length of the Service name from
contributing to the overall width. Your new (e) example would
look
like:
root@S11:~# installadm list -m
Service/Manifest Name Status Criteria
--------------------- ------ --------
default-i386
orig_default Default (Ignored: ipv4 = 129.168.1.2)
(Ignored: mem = 2048 MB -
unbounded)
default-sparc
orig_default Default None
new_inactive_manifest Inactive None
solaris11-i386
orig_default arch = i86pc
mac = AA:BB:CC:DD:EE:FF
ipv4 = 10.0.2.100 -
10.0.2.199
cpu = i386
mem = 1024 MB
network = 10000000000
platform = i86pc
zonename = some-zone-name-1
some-new_manifest-1 arch = sparc
mac = AA:BB:CC:DD:EE:EE
mem = 1024 MB
some-new_manifest-2bbbbbbb Default None
solaris11-sparc orig_default Default None
We used this approach for "beadm list [-d] [-s]".
-ethan
On 11/29/11 01:15, Tomas Dzik wrote:
Hi Dave,
thanks a lot for your comment. So taking into account that
criteria
themselves could be quite long (only mac=AA:BB:CC:DD:EE:EE
without
spaces around '=' occupy 21 characters) do you (or anyone
else on
this
mailing list) have some suggestion how the output should look
like ?
Best regards,
Tomas D.
Dne 28.11.11 16:41, Dave Miner napsal(a):
Tomas, thanks for taking this on, it's an important
improvement.
But,
can we work out an output format that will keep the width at 80
columns
more often than not? The examples seem to be wider than that in
general,
and I'd expect complaints about readability as shown.
Dave
On 11/24/11 09:36, Tomas Dzik wrote:
Hi all,
I would like to ask you for a code-review for:
7036455 "installadm list -m" should show criteria
Webrev:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7036455
How it works:
a) It prints for "installadm list -m" similar output like for
"installadm list -m -n svcname"
b) For each service it prints active manifests + their
associated
criteria
fist, followed by default manifest (+ plus it's associated
criteria
marked as Ignored),
followed by inactive manifests. In each of that categories
order of
manifest is not
explicitly set
c) For each manifest criteria are printed in given order.
Order is
exactly the same like
if you run "installadm list -m -n svcname"
d) For examples of output, please look at "Testing" paragraph.
Testing:
1) I run all unit tests using ./slim_test -c tests.nose and
there
were
no regressions
2) I run pep8 on file which I modified and there were no
issues
3) I created 2 x86 virtual machines and I installed my built
workspace
on one of them
and let the second-one for reference. Then I created several
services
with different manifests
using different criteria. Here are the original outputs of
installadm
list compared to outputs
after my fix:
a) Original:
root@S11:~# installadm list -m
Service Name Manifest Status
------------ -------- ------
default-i386 orig_default Default
default-sparc orig_default Default
solaris11-i386 orig_default Default
solaris11-sparc orig_default Default
a) New:
root@S11:~# installadm list -m
Service Name Manifest Status Criteria
------------ -------- ------ --------
default-i386 orig_default Default None
default-sparc orig_default Default None
solaris11-i386 orig_default Default None
solaris11-sparc orig_default Default None
b) Original:
root@S11:~# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default None
b) New:
root@S11:~# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default None
c) Original:
root@S11:~# installadm list -m
Service Name Manifest Status
------------ -------- ------
default-i386 orig_default Default
default-sparc orig_default Default
solaris11-i386 orig_default Default
solaris11-sparc orig_default Default
c) New:
root@S11:~# installadm list -m
Service Name Manifest Status Criteria
------------ -------- ------ --------
default-i386 orig_default Default (Ignored: ipv4 =
192.168.1.2)
(Ignored: mem = 2048 MB
- unbounded)
default-sparc orig_default Default None
solaris11-i386 orig_default Default None
solaris11-sparc orig_default Default None
d) Original:
root@S11:~# installadm list -m
Service Name Manifest Status
------------ -------- ------
default-i386 orig_default Default
default-sparc orig_default Default
solaris11-i386 some-new_manifest-2bbbbbbb Default
solaris11-sparc orig_default Default
d) New:
root@S11:~# installadm list -m
Service Name Manifest Status Criteria
------------ -------- ------ --------
default-i386 orig_default Default (Ignored: ipv4
= 129.168.1.2)
(Ignored: mem
= 2048 MB - unbounded)
default-sparc orig_default Default None
solaris11-i386 some-new_manifest-2bbbbbbb Default None
orig_default Inactive None
some-new_manifest-1 Inactive None
solaris11-sparc orig_default Default None
e) Original:
oot@S11:~# installadm list -m
Service Name Manifest Status
------------ -------- ------
default-i386 orig_default Default
default-sparc orig_default Default
solaris11-i386 orig_default
some-new_manifest-1
some-new_manifest-2bbbbbbb Default
solaris11-sparc orig_default Default
e) New:
root@S11:~# installadm list -m
Service Name Manifest Status Criteria
------------ -------- ------ --------
default-i386 orig_default Default (Ignored: ipv4
= 129.168.1.2)
(Ignored: mem
= 2048 MB - unbounded)
default-sparc orig_default Default None
new_inactive_manifest Inactive None
solaris11-i386 orig_default arch = i86pc
mac =
AA:BB:CC:DD:EE:FF
ipv4 =
10.0.2.100 - 10.0.2.199
cpu = i386
mem = 1024 MB
network =
10000000000
platform = i86pc
zonename =
some-zone-name-1
some-new_manifest-1 arch = sparc
mac =
AA:BB:CC:DD:EE:EE
mem = 1024 MB
some-new_manifest-2bbbbbbb Default None
solaris11-sparc orig_default Default None
4) Because I modified function which is also used for printing
profiles
I checked
that listing of profiles didn't change
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
