Merge ~smoser/cloud-init:bug/1675185-no-apt-on-snappy into cloud-init:master

Proposed by Scott Moser
Status: Merged
Merged at revision: e80dbb80987ba44be2899e34fbbbf7d48389b6b5
Proposed branch: ~smoser/cloud-init:bug/1675185-no-apt-on-snappy
Merge into: cloud-init:master
Diff against target: 37 lines (+17/-3)
1 file modified
cloudinit/config/cc_apt_configure.py (+17/-3)
Reviewer Review Type Date Requested Status
Ryan Harper Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+321218@code.launchpad.net

Commit message

apt_configure: run only when needed.

Do not bother configuring apt if no 'apt' config is provided and either:
 a.) running on snappy
 b.) there is no 'apt' command (possibly a different distro)

If apt config is provided in either of the above situations, then config
will continue.

LP: #1675185

To post a comment you must log in.
Revision history for this message
Scott Moser (smoser) wrote :

need some tests, and maybe some test case cleanups.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) :
review: Needs Fixing
Revision history for this message
Scott Moser (smoser) wrote :

The first thing that apply_debconf_selections does is:
    selsets = cfg.get('debconf_selections')
    if not selsets:
        LOG.debug("debconf_selections was not set in config")
        return

So if there is no 'debconf_selections' in the config, then it will just log and return.
If there *is* debconf_selections in the config, then it seems like the proper thing to do is fail trying.

That is slightly different than 'apt' configuration for 2 reasons
a.) apt config is more likely to just be there anyway (such as maas sending it)
b.) there is some "default" apt configuration that we do based on system config.

That said, I think that its probably best for us fail if 'apt' config is specifically given and we are on snappy or there is no 'apt'.

I'm going to update the proposal here to fail if apt config is given.
thoughts?

Revision history for this message
Ryan Harper (raharper) wrote :

I personally like an explicit disable; we have it in many other modules; it's simple and clean; even the debconf_selections does that (as you point out).

I agree that if someone puts in debconf_selections or an apt config in user-data, then we should fail; I'm fine with the apt config present failing if there's no apt or system_is_snappy.

Can we do both? (explicit config disable and checking for apt/snappy)?

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

Looks good

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py
index 7f09c91..06804e8 100644
--- a/cloudinit/config/cc_apt_configure.py
+++ b/cloudinit/config/cc_apt_configure.py
@@ -278,15 +278,29 @@ def handle(name, ocfg, cloud, log, _):
278 raise ValueError("Expected dictionary for 'apt' config, found %s",278 raise ValueError("Expected dictionary for 'apt' config, found %s",
279 type(cfg))279 type(cfg))
280280
281 LOG.debug("handling apt (module %s) with apt config '%s'", name, cfg)281 apply_debconf_selections(cfg, target)
282 apply_apt(cfg, cloud, target)
283
284
285def apply_apt(cfg, cloud, target):
286 # cfg is the 'apt' top level dictionary already in 'v3' format.
287 if not cfg:
288 # no config was provided. If apt configuration does not seem
289 # necessary on this system, then return.
290 if util.system_is_snappy():
291 LOG.debug("Nothing to do: No apt config and running on snappy")
292 return
293 if not (util.which('apt-get') or util.which('apt')):
294 LOG.debug("Nothing to do: No apt config and no apt commands")
295 return
296
297 LOG.debug("handling apt config: %s", cfg)
282298
283 release = util.lsb_release(target=target)['codename']299 release = util.lsb_release(target=target)['codename']
284 arch = util.get_architecture(target)300 arch = util.get_architecture(target)
285 mirrors = find_apt_mirror_info(cfg, cloud, arch=arch)301 mirrors = find_apt_mirror_info(cfg, cloud, arch=arch)
286 LOG.debug("Apt Mirror info: %s", mirrors)302 LOG.debug("Apt Mirror info: %s", mirrors)
287303
288 apply_debconf_selections(cfg, target)
289
290 if util.is_false(cfg.get('preserve_sources_list', False)):304 if util.is_false(cfg.get('preserve_sources_list', False)):
291 generate_sources_list(cfg, release, mirrors, cloud)305 generate_sources_list(cfg, release, mirrors, cloud)
292 rename_apt_lists(mirrors, target)306 rename_apt_lists(mirrors, target)

Subscribers

People subscribed via source and target branches