Dave,
The update looks fine.
One last thing I forgot to comment on was to update all Copyrights to 2012.
-ethan
On 01/04/12 14:51, Dave Miner wrote:
On 01/03/12 20:04, Ethan Quach wrote:
On 01/03/12 13:27, Dave Miner wrote:
On 12/29/11 18:46, Ethan Quach wrote:
Dave,
cgi_get_manifests.py - 275 - I don't think running the value through
formatValue() is the right thing to do here for all criteria. For the
memory criteria at least, that function returns a human readable
format
(appending " MB" to memory value) which wouldn't seem to be
appropriate.
Looking at the existing code, it seems the code at
create_profile.py:394-395 is the closest clue of what we need to do
here. That code read criteria values from the DB (which is, barring
the
hex x'mac' value) is equal to what is coming in the postData
request at
install time. This code is only calling formatValue() for range
criteria, so perhaps at a minimum, we need to do that here as well.
Thanks, I hadn't noticed the issue with mem, but it's the only one
where the wrong thing would happen. The solution I came up with is to
just add an optional argument to formatValue to control whether the
units are appended. All of the existing consumers get what they
already expect and in this case we ensure the value that comes back is
just the criteria without the units.
Sounds OK.
common_profile.py - 47 - perhaps a comment noting that the values in
this dict are used only to do upfront validation of profiles with
variables?
Accepted
common_profile.py - 91 - If we initialize with this here, we'd
potentially use one of those values from the hard-coded dict as a
replacement value (if the ENV didn't have that criteria var set.) This
could actually happen when the client is older than the server, and is
not sending over some new supported criteria on the server. In such a
case, perhaps it'd be more appropriate to leave whatever wasn't set
un-substituted or blanked, and then allow subsequent validation to
fail.
Perhaps make the function assume that whatever substitution values the
caller wants to use are already set in the ENV, and then make
create_profile.py set the ENV with the hard-coded dict before calling
validate_file()?
Hmm, I hadn't considered the case of new criteria with old clients. I
wasn't real keen on the way this was using the environment, anyway,
but had decided I didn't have a compelling reason to change it. Your
comment coupled with our decision to remove the stuff below means
we're better off not using the environment and instead should just
optionally pass in a dictionary to perform_templating.
Yes, this sounds like a better approach.
cgi_get_manifest can do that, the other cases can all just let the
default dict be used (except unit tests, which I've updated). This
also allows perform_templating to just drop the whole loop where it's
trying to validate values, since we don't have any arbitrary user
input for it to worry about.
Indeed.
You'll note that I kept the upper-casing of the MAC address,
This would seem unnecessary if the input dict is never going to be
arbitrary anymore, but I suppose it doesn't hurt anything. One question
though, if AI_MAC wasn't in the provided dict, wouldn't this traceback?
Yes, it would, I've fixed.
Note that cgi_get_manifest.py::send_manifest(), which does the
processing of criteria from the postData, does support sparse criteria
in the postData string as can be seen in the unit test
test/test_cgi_manifest.py::testSendManifest(). That unit test's formData
happens to have a mac criteria though.
A real live test that could be run to exercise this is to install system
with a zone, and provide the zone a profile from the install service. Or
perhaps you could add a new test function in that class to mimic the
postData when a request is made for a zone? (When auto-install queries
the install service for zone manifests and profiles, it only sends over
the 'zonename' criteria in the postData; admittedly, such a test
function with that specific sparseness probably should have been added
when the zones work was implemented :-\ )
I modified the template tests to include a specific case for zone only.
A differential for the above is up at
https://cr.opensolaris.org/action/browse/caiman/dminer/slim_7124655-diff/webrev/
There's a bunch of extra noise there to get rid of trailing spaces
that were annoying pep8.
Dave
but I removed the special stuff for client ID, as usage of that has
not been documented and it seemed broken as implemented, anyway.
This sounds fine to me. The only references I found of it from the
original SCProfiles design document
(http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/SCProfileProposal.odt)
were example usages where it was used as the variable to dynamically map
XInclude snippets per client, or to dynamically lookup values using a
script with Cheetah templates, neither of which we ended up
implementing.
At any rate, it would seem like using AI_MAC would be just as adequate
for those usages.
create_profile.py - 261-307 - These lines can be removed now since we
don't really care to set the criteria passed in from commandline into
the environment anymore.
create_profile.py - 377-401 - Same here.
Both accepted.
Revised webrev at same location
https://cr.opensolaris.org/action/browse/caiman/dminer/slim_7124655/webrev/
Modulo the comment above, the updated webrev looks good to me.
thanks,
-ethan
Dave
thanks,
-ethan
On 12/28/11 13:55, Dave Miner wrote:
I'd appreciate review of the changes for
7124655 Profile template expansion should happen at installation time
Webrev is at:
https://cr.opensolaris.org/action/browse/caiman/dminer/slim_7124655/webrev/
This has been tested successfully with both SPARC and x86 clients and
servers, updated unit tests are clean.
Note that this change will require updates to the installadm man page
and the profile docs to reflect the deferred application of
templating.
Dave
_______________________________________________
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
