Skip to content

Conversation

@PhillSimonds
Copy link
Contributor

@PhillSimonds PhillSimonds commented Sep 16, 2020

This PR addresses the following issues:

  • The onboarding_extensions_map setting (specified in netbox_onboarding/init.py
    or overridden in configuration.py) is used to map napalm driver names to a custom
    class which extends the driver, allowing extensibility. Currently, when a mapping
    doesn't exist for a napalm driver, the NetdevKeepr class's get_onboarding_facts()
    method fails. This causes the rq-worker to be unable to run the onboard_device()
    function to onboard a device. The changes in this commit fix the issue.

  • Logging is not instantiated in the same way from file to file, and thus log messages
    don't come through (at all in some cases) or consistently (in others). This PR standardizes
    logging such that logs are consistently displayed while troubleshooting

  • Currently, the template_content.py DeviceContent class returns None if onboarding is
    not enabled for an OnboardingDevice object. Likewise, if no OnboardingDevice object
    exists, the template continues trying to access attributes for an OnboardingDevice object.
    In the first case, template rendering will fail as an empty string is needed in order to insert
    nothing into the rendered HTML template presented to the user. In the second case, an
    AttributeError is raised as you can not access attributes of a NoneType object. This PR returns
    empty strings in both cases to ensure the HTML renders correctly.

The onboarding_extensions_map setting (specified in netbox_onboarding/__init__.py
or overridden in configuration.py) is used to map napalm driver names to a custom
class which extends the driver, allowing extensibility. Currently, when a mapping
doesn't exist for a napalm driver, the NetdevKeepr class's get_onboarding_facts()
method fails. This causes the rq-worker to be unable to run the onbaord_device()
function to onboard a device. The changes in this commit fix the issue.
@dgarros
Copy link
Contributor

dgarros commented Sep 17, 2020

Also @PhillSimonds please can check if we need to fix the logger on other files, would be good to do all at once

Currently, the template_content.py DeviceContent class returns `None`
if onboarding is not enabled for an OnboardingDevice object. Likewise,
if no OnboardingDevice object exists, the template continues trying to
access attributes for an OnboardingDevice object. In the first case,
template rendering will fail as an empty string is needed in order
to insert nothing into the rendered HTML template presented to the user.
In the second case, an AttributeError is raised as you can not access
attributes of a NoneType object.
@glennmatthews
Copy link
Contributor

Since you're fixing the logging, consider enabling logging for rq.worker in development/base_configuration.py. Something like:

LOGGING = {
    "version": 1,
    "disable_existing_loggers": False,
    "formatters": {"rq_console": {"format": "%(asctime)s %(message)s", "datefmt": "%H:%M:%S",},},
    "handlers": {
        "rq_console": {
            "level": "DEBUG",
            "class": "rq.utils.ColorizingStreamHandler",
            "formatter": "rq_console",
            "exclude": ["%(asctime)s"],
        },
    },
    "loggers": {"rq.worker": {"handlers": ["rq_console"], "level": "DEBUG",}},
}

@PhillSimonds
Copy link
Contributor Author

Cool! I'll throw something in!

@mzbroch mzbroch self-requested a review September 18, 2020 14:24
@PhillSimonds PhillSimonds force-pushed the psi-20200916 branch 4 times, most recently from 237fd18 to 3ccab16 Compare September 21, 2020 14:10
@dgarros
Copy link
Contributor

dgarros commented Sep 21, 2020

@glennmatthews @mzbroch Anything else missing before we can merge this one ? it would be useful to get a beta2 ASAP with these fixes.
My recommendation would be to add more unit tests in a separate PR.

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, will keep an eye out for a follow-on PR with unit test improvements. :-)

Copy link
Contributor

@mzbroch mzbroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mzbroch mzbroch merged commit fa9e365 into develop-2.0 Sep 22, 2020
@mzbroch mzbroch deleted the psi-20200916 branch September 22, 2020 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants