Client must import configuration for standalone server

This is great, +1

+ help="Filename or URL to import configuration from.")

I'd make it clear that it only imports the initial configuration from there, as a "bootstrap" mechanism.

Other than that, it's great :-)

Guys, please, set a milestone when opening a ticket.

Well done, and the landscape/lib/fetch.py:fetch() was definitely needing some error handing :)

+1

[1]
- args, accept_unexistent_config=accept_unexistent_config)
+ args, accept_nonexistent_config=accept_nonexistent_config)

Thanks :-)

[2]
+ def __init__(self, fetch_import_url=None):
+ super(LandscapeSetupConfiguration, self).__init__()
+ if fetch_import_url is None:
+ self._fetch_import_url = self._undefined_fetch_import_url
+ else:
+ self._fetch_import_url = fetch_import_url
+
+ def _undefined_fetch_import_url(self, url):
+ raise RuntimeError("fetch_import_url not defined")

This code seems unnecessary and indeed it's untested. If I just change it to unconditionally say "self._fetch_import_url = fetch_import_url", all the tests still pass.

[3]
+ def _load_external_options(self):
+ if self.import_from:
+ parser = ConfigParser()
+
+ # Imported options behave as if they were passed in the
+ # command line, with precedence being given to real command
+ # line options.

That comment seems like it'd be better in a docstring or even in the --help for the option, since it's actually something that's important for the user to know.

[4]
+ if config.ssl_public_key and config.ssl_public_key[:7] == "base64:":

There's a neat new feature in python 1.6 called str.startswith ;-)

[5] Could you add a test for importing an URL with outright syntax errors? Like, for example, if someone tries to --import http://google.com/, what would happen?

[6] This does seem to be handled correctly, but could you add an explicit test that when "nulling out" a value in the imported configuration file, it correctly saves the empty value? i.e. if old config has "registration_password = foo" and imported configuration has "registration_password = ", then the new config should have "" as the value for registration_password.

This is looking good! +1 with these changes.

Also, I think we should get Andreas to take a crack at this branch before we merge it.

[Kevin]

[1]

Cool, I've addressed this with Chris' review below.

[Free]

Thanks!

[Chris]

[1]

No problem. :-)

[2]

My goal was to have a way to both prevent it from being left unset to something sensible by mistake (IOW, I didn't want to have fetch() as a default), and to prevent having to pass the parameters explicitly at all times. In any case, it's bogus that it had no tests (it would break anyway, but might break with a weird NameError or something), and the concept was also awkward no matter what. I'm now enforcing the parameter to be passed in.

[3]

Done, I've added that to both places. This should also sort Kevin's comment.

[4]

Oops.. changed. :-)

[5]

Good point! That's fixed!

[6]

Test added!

Thanks for the great review!

qa + 1

This bug was fixed in the package landscape-client - 1.0.28-0ubuntu1.9.04.0

---------------
landscape-client (1.0.28-0ubuntu1.9.04.0) jaunty; urgency=low

  * Fix minor packaging issues in last release (LP: #343954)
    - Version number in landscape.VERSION is now correct
    - Fixed package version number to maintain convention
  * The following changes are in the 1.0.28 release:
    - Invalidate package cache when server UUID changes (LP: #339948)
    - Improve the "cloud mode" introduced in 1.0.26 to send more
      disambiguation data (LP: #343942) and allow the EC2 user data to specify
      the exchange and ping URLs (LP: #343947)
    - Allow importing of initial configurations (along with public SSL
      certificates) when running landscape-config (LP: #341705)
    - Support a non-root mode which allows running the client without the
      management functionality (LP: #82159)
    - Automatic cloud registration when there's no user-data to specify an OTP
      now works (LP: #344323)

 -- Christopher Armstrong <email address hidden> Thu, 19 Mar 2009 09:52:03 -0400