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

Newer junos versions return a bool for config diff if there are no changes #1093

Closed
wants to merge 1 commit into from

Conversation

dangoscomb
Copy link

Check return type is an etree Element, if not return None for no changes. Fixes #1082

@dangoscomb dangoscomb changed the title Newer junks versions return a bool for config diff if there are no changes Newer junos versions return a bool for config diff if there are no changes Jan 4, 2021
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 94.135% when pulling 9efef02 on dangoscomb:fix-diff into fc5debc on Juniper:master.

@alexandrecorso
Copy link

It looks like the CI is blocked with some crypto package not about the fix.

@ntwrkguru
Copy link
Contributor

Looks like some openssl headers are missing from the CI runner.

build\temp.win-amd64-3.7\Release\_openssl.c(575): fatal error C1083: Cannot open include file: 'openssl/opensslv.h': No such file or directory

@hhopjh
Copy link

hhopjh commented May 7, 2021

Any chance to implement this fix?

@rahkumar651991
Copy link
Contributor

Closing this as it is junos bug for certain releases and should work fine for newer releases.
The suggested fix will work for these releases but is a workaround and is not recommended as it will let us allow multiple error-scenario as successful-scenario.

@dkasten79
Copy link
Contributor

I don't agree with closing this as this will break automation for anyone using PyEZ just because of the release of Junos they are using. It may be a bug with Junos but the bug shouldn't break PyEZ. The fix should be to check for if a boolean was returned and then test if it is true and then return None.

There are going to be people using Junos releases with this bug and they shouldn't have to make manual modifications of PyEZ to get it to work. Currently I am running in to this for a Juniper customer and it broke their automation and took me a whole day to figure out what was going on and why the automation broke.

@rahkumar651991
Copy link
Contributor

@dkasten79 - If a safe-check net is to be added for this scenario, we can discuss it.
I will re-open the issue and we can analyse and add check regarding it. But, this pull request will create problem with other scenario and it will not be backward-compatible.

I am re-opening the Issue #1082. Will analyse for more solutions. Might need help to get some desired outputs from devices throwing the error.

@dkasten79
Copy link
Contributor

I have an internal Juniper VM environment setup with gitlab, gitlab-runner, and AWX that is running in to this problem but I put in a manual fix currently to resolve the issue in my testing environment. Once I am done with using the lab for customer work I can provide access for you to test and see outputs from the failure scenario.

If you want to discuss the fix and what issues this error handling could cause and why it wouldn't be backward compatible then you can message me internally at dkasten.

@dangoscomb
Copy link
Author

Happy to provide any outputs required. I fail to see what "multiple error scenario" is not covered at a quick glance, let me know what I'm missing.

dkasten79 added a commit to dkasten79/py-junos-eznc that referenced this pull request Jun 2, 2021
@rahkumar651991
Copy link
Contributor

@dangoscomb @dkasten79 -
Discussed this with the team. This is a specific bug at Junos and it would be difficult if we start adding workarounds for specific versions having bugs.

We do agree that we should return error-message or raise some warning instead of throwing system errors. Also, noticed that another pull-request 1116 has been raised for the same. Let us review the same and we will get back with the resolution soon.

@dangoscomb
Copy link
Author

I'm not sure the bug is in a "specific version". It seems to affect many to me. Do you have the actual PR reference? I haven't noticed it anywhere.

From my point of view, upgrading JUNOS for this issue alone will affect many thousands of users... adding a workaround to a management system will affect no one.

My approach would be to fix, where safe to do so (and I believe the submitted fix is safe) to allow for the same error being re-introduced in future. Its great to assume that JUNOS will always be error free and I really wish this was going to be the case, believe me, but making the management system handle errors (even if not intended) is far more important IMO.

rahkumar651991 added a commit that referenced this pull request Sep 9, 2021
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.

Config.diff() issue with rsp type
7 participants