Skip to content

Juniper junos show system configuration database usage #2087

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jnicholson56
Copy link
Contributor

Adding new file juniper_junos_show_system_configuration_database_usage.textfsm.

Send PR as draft for feedback before finalizing it.

@mjbear
Copy link
Collaborator

mjbear commented Mar 24, 2025

@jnicholson56
Should the megabyte (MB) unit abbreviation be captured?
Reworded: Should we also capture the megabyte (MB) unit abbreviation?

Are those units always in megabytes?
Could those ever be in gigabytes (GB)?
(Food for thought possibly.)

In the meanwhile, I do have some whitespace regex suggestions I'll start putting out there.

@jnicholson56
Copy link
Contributor Author

jnicholson56 commented Mar 24, 2025

@jnicholson56 Should the megabyte (MB) unit abbreviation be captured? Reworded: Should we also capture the megabyte (MB) unit abbreviation?

Are those units always in megabytes? Could those ever be in gigabytes (GB)? (Food for thought possibly.)

In the meanwhile, I do have some whitespace regex suggestions I'll start putting out there.

In all the examples I have on my network (100's), they are all labeled as MB. The output is the same on both Classic and EVO as well.

Thanks for the suggestions. I had a few mistakes that I have committed as well. Hopefully tests pass this time :D

@jnicholson56 jnicholson56 marked this pull request as ready for review March 24, 2025 23:47
@jnicholson56
Copy link
Contributor Author

This is ready to go unless there are more suggestions.

@mjbear
Copy link
Collaborator

mjbear commented Mar 24, 2025

In all the examples I have on my network (100's), they are all labeled as MB. The output is the same on both Classic and EVO as well.

And now that I look at the raw cli output again, I do notice some values are more than a thousand and are still in MB units.

All good. 😀 😅

@jnicholson56
Copy link
Contributor Author

Juniper is nothing if not inconsistent across the CLI :)

@mjbear mjbear added the status: action required This issue requires additional information to be actionable label Apr 4, 2025
@jnicholson56
Copy link
Contributor Author

jnicholson56 commented Apr 4, 2025 via email

@mjbear mjbear added question and removed status: action required This issue requires additional information to be actionable labels May 6, 2025
@jnicholson56
Copy link
Contributor Author

jnicholson56 commented May 8, 2025 via email

@mjbear
Copy link
Collaborator

mjbear commented May 8, 2025

Originally this template was failing when the configuration was being updated. The configuration database gets locked and can't be polled. I added the error as a way to let me know it failed for that reason and that I could ignore the issue.

Gotcha

There may be a better way to handle it but this worked for me in my ignorance.

I'll make suggestion(s) and you can decide.

If the config db was locked the result would be an empty list which is easy to test for in Python.

---
parsed_sample: []

(An empty list evaluates to the boolean False in Python.)
🥳 I'd go with this solution if it were me. 😅

@jnicholson56
Copy link
Contributor Author

jnicholson56 commented May 10, 2025 via email

@mjbear
Copy link
Collaborator

mjbear commented May 10, 2025

My original solution for the problem a few years back now. I am using netconf to poll and the model is a bit more forgiving. If you want I can remove this error from the template.

How about keeping the message in the template, but remove the Error action?
(I will submit a suggestion on GH here.)

(Ultimately since nothing matches we end up with an empty Python list which is easy to test for in the code that will use the list of parsed data.)

Since this has been dangling for some time I suspect you guys want to get it completed and pushed through.

Things got busy -- breaking changes were held to the side for a major release and normal changes piled up a bit too. 😀 I'm looking to merge a few to reduce our number of remaining PRs. 🙂

@jnicholson56
Copy link
Contributor Author

@mjbear That works for me.

- Remove the Error directive from the "currently being modified" line
- Add test data for the "currently being modified" cli output
@mjbear
Copy link
Collaborator

mjbear commented May 13, 2025

@mjbear That works for me.

My commit message is a bit off - it was from my testing with that line in a test output file.

I have since pushed the test output file (with the config db locked message) to this branch so we have test coverage.

Apologies for any confusion ... I should have amended my commit message. 😐

@mjbear mjbear removed the question label May 17, 2025
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.

2 participants