no need to use import_opt or import_group if we already import that module

Bug #1389229 reported by ZhiQiang Fan
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ceilometer
Fix Released
Low
ZhiQiang Fan

Bug Description

import_opt and import_group will import specified module, which will invoke opt or group registeration, then it try to get specified opt or group from registered options, if fails, NoSuchOpt exception will be raised.

Service can run properly because every service will import ceilometer.service. But if we do not import those opt in the import chain, unit test will be blocked when we run individual test by nosetest (tox seems fix this issue automatically)

IMHO, a common case should be avoid, which is that: we already import that module, and then use cfg.CONF.import_opt|group() in that module, that is bad for obviously redundant. it is not acceptable.

But, if it is not so clear, for example, the import or register hides in the middle or the end of the import chain, then duplicate import_opt|group is acceptable, because it is hard for developer to track the import chain just to avoid redundant, and such check will add the burden for reviewers, and maintainers.

A hacking rule may be the best choice, but I'll handle it manually for now

ZhiQiang Fan (aji-zqfan)
Changed in ceilometer:
assignee: nobody → ZhiQiang Fan (aji-zqfan)
Revision history for this message
gordon chung (chungg) wrote :

hi ZhiQiang, please reopen, but i don't think this is a ceilometer bug... just a preference... i'm marking as invalid but reopen if you think otherwise

Changed in ceilometer:
status: New → Incomplete
status: Incomplete → Won't Fix
ZhiQiang Fan (aji-zqfan)
description: updated
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to ceilometer (master)

Fix proposed to branch: master
Review: https://review.openstack.org/136081

Changed in ceilometer:
status: Won't Fix → In Progress
Eoghan Glynn (eglynn)
Changed in ceilometer:
milestone: none → kilo-2
importance: Undecided → Low
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ceilometer (master)

Reviewed: https://review.openstack.org/136081
Committed: https://git.openstack.org/cgit/openstack/ceilometer/commit/?id=d288a28589d3791bc71eee626d1ed0b7ac7bcd23
Submitter: Jenkins
Branch: master

commit d288a28589d3791bc71eee626d1ed0b7ac7bcd23
Author: ZhiQiang Fan <email address hidden>
Date: Fri Nov 21 01:58:45 2014 +0800

    Remove unnecessary import_opt|group

    Usually, we should import an option before we use it, otherwise,
    NoSuchOpt exception will be raised.

    When we import a module, the options in that module are registered,
    (or imported), so we don't need to explicitly import them again.
    However, it is a bit hard for developers, reviewers and maintainers
    to track between files just to check if a specific option is imported
    or not. So it is good (not required) whenever we use an option, we import
    it explicitly, such kind of redundant is acceptable. But not the case that
    we already import a module A, then import_opt|group registered or imported
    in that module again, this is obviously unnecessary and should be avoid.

    Change-Id: I8672d9f12d862f9a622e5557d9c8dc2fe6717f5e
    Closes-Bug: #1389229

Changed in ceilometer:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in ceilometer:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in ceilometer:
milestone: kilo-2 → 2015.1.0
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.