fix: register nac, secureConnect, support, users, and assistant categories - #485
Open
priyavrat7 wants to merge 1 commit into
Open
fix: register nac, secureConnect, support, users, and assistant categories#485priyavrat7 wants to merge 1 commit into
priyavrat7 wants to merge 1 commit into
Conversation
…ories These five categories existed as fully working generated code under meraki/api/ and meraki/aio/api/, but these were not added them to meraki/__init__.py or meraki/aio/__init__.py. Since that file is the one place that actually wires a category onto DashboardAPI, calling dashboard.nac (or any of the other four) just raised AttributeError even though the code behind it worked fine on its own. I found this while looking into the nac report specifically, but once I checked what else might be missing the same wiring, I found four more categories in the exact same state. Fixed all five here since it is the same two line pattern each time. Also extended test_all_api_sections_initialized so these five are actually checked going forward, since the original version only checked 5 of the (now) 22 registered categories and that is basically how this slipped through in the first place. Fixes meraki#472
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #472
The issue reported that nac is not accessible on DashboardAPI, even though the underlying methods exist and work fine on their own. I traced this to meraki/init.py, which is a hand maintained file, not generated, and is responsible for importing each category class and attaching it to the client. nac was generated under meraki/api/ but never wired into this file.
While checking this, I compared every file under meraki/api/ against what is actually registered in meraki/init.py and found the same gap for four more categories, secureConnect, support, users, and assistant. All five exist as fully working generated code, none were wired up. I found the same issue in the async client, meraki/aio/init.py, and fixed it there too.
I decided to fix all five in this one PR instead of just nac, since it is the same root cause and the same two line fix repeated five times, not five separate bugs. That said, if the team would rather keep this PR scoped to just nac and handle the other four as a separate issue and PR, I am completely fine doing that instead. Happy to split it however works best on your end.
What I think is not in scope here
test_all_api_sections_initialized previously checked only 5 of the 22 registered categories. I extended it to also check the 5 I fixed here, so it now checks 10 of 22. I left the remaining 12 uncovered on purpose, since those were never broken and adding coverage for working code is a separate improvement, not part of this bug fix. I can open a follow up issue for that if it is useful.
There is currently no unit test anywhere in this repo for AsyncDashboardAPI initialization, only integration tests that need a live org. So the async fix here does not have a matching unit test. Wanted to flag that honestly instead of skipping it quietly or building new test infrastructure as part of this PR.
Testing
Reproduced on a clean beta checkout, all five raised AttributeError on both DashboardAPI and AsyncDashboardAPI before the fix
Confirmed all five resolve correctly after the fix, on both clients
Extended test_all_api_sections_initialized and confirmed it fails without the fix with AttributeError, DashboardAPI object has no attribute nac, then passes with it
Also manually confirmed the 12 untouched categories still work fine, on both clients, so this fix does not touch anything outside the five it is meant to
Full unit suite passes, 211 passed
ruff check and ruff format are clean on both edited files aside from a few pre existing issues, confirmed with git blame to predate this change