Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre decorator commit #540

Closed

Conversation

itdependsnetworks
Copy link
Contributor

ok, this is a mess, but it's a start.

I had troubles making it a decorator, so I wanted to get something on paper to agree on, and perhaps some pointers.

@coveralls
Copy link

coveralls commented Nov 19, 2017

Coverage Status

Coverage decreased (-0.7%) to 77.05% when pulling a286f3e on itdependsnetworks:logging into 136c335 on napalm-automation:develop.

@dbarrosop
Copy link
Member

What is it you are trying to achieve here?

@itdependsnetworks
Copy link
Contributor Author

@dbarrosop
Copy link
Member

Ok, so if we want to go with the recorder/replayer I think we should continue the work in napalm-automation/napalm-base/pull/296.

If you just want to log for posterity what's going on do instead:

import logging

log_backend = logging.getLogger("napalm-backend")

...

    def __send_command(self, command):
    ...
        output = __send_command(self, command)
        log_backend.debug(command)    

Way simpler and cleaner and then you can just enable it and configure it by just configuring the logger.

@itdependsnetworks
Copy link
Contributor Author

Logging in that matter does not accomplish my goals. See my comment in PR 296.

Reading the prior implementation PR, it is unclear to the reader (or at least me) what was discussed on slack and what the context of that convo was. Picking up this work seems like a much larger proposition, when in reality I was only selfishly concerned with the recording feature of it.

@dbarrosop
Copy link
Member

well, if you want the recording capability we should continue the work previously started. This solution here is very adhoc and will probably lead to mistakes and inconsistencies.

@itdependsnetworks
Copy link
Contributor Author

@dbarrosop

I don't know what was decided, so there is nothing for me to continue.

@dbarrosop
Copy link
Member

I think we should resend the recording/replayer PR to napalm and then take it from there. I will try to do that this week and ping you so you can comment and provide feedback/ideas to make sure it meets your needs.

@dbarrosop dbarrosop closed this Dec 12, 2017
@dbarrosop
Copy link
Member

Closing, as usual feel free to comment/reopen if you don't agree with my decision or if you have something else to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants