Performance: avoid loading expensive modules until needed #2871
Conversation
…e Profile without having to load adal and friends
…ge of pprint.pformat)
…e (delay loaded a bit too much)
…ethod (cleaner code)
Codecov Report
@@ Coverage Diff @@
## master #2871 +/- ##
==========================================
- Coverage 63.19% 63.19% -0.01%
==========================================
Files 466 466
Lines 26161 26168 +7
Branches 4002 4003 +1
==========================================
+ Hits 16533 16536 +3
- Misses 8545 8548 +3
- Partials 1083 1084 +1
Continue to review full report at Codecov.
|
tjprescott
left a comment
tjprescott
left a comment
There was a problem hiding this comment.
LGTM but I think @yugangw-msft should review it as well, especially since it's not covered by any tests (AFAIK).
yugangw-msft
left a comment
yugangw-msft
left a comment
There was a problem hiding this comment.
Suggest 1 more change. The rest LGTM
| subscriptions = [] | ||
| if interactive: | ||
| subscriptions = self._subscription_finder.find_through_interactive_flow( | ||
| subscriptions = self.subscription_finder.find_through_interactive_flow( |
There was a problem hiding this comment.
This is an existing code defect, but since you are improving around, maybe you would not mind going one step further:
- Let us get rid of the instance field of
subscription_findercompletely since it is only being used in this routine. - For test seam injection, let us have this method take an option argument, so impacted unit test can pass in a stub. Note, your PR is already touching that test, so the code change is trivial.
There was a problem hiding this comment.
@yugangw-msft, that's a good suggestion. Done.
| return deepcopy(consolidated) | ||
|
|
||
| @property | ||
| def subscription_finder(self): |
There was a problem hiding this comment.
And we don't need this one any more if you agree with my previous comment
| self._auth_ctx_factory = auth_ctx_factory or _AUTH_CTX_FACTORY | ||
| self.adal_token_cache = None | ||
| self._load_creds() | ||
| self._adal_token_cache_attr = None |
There was a problem hiding this comment.
I don't think we need _attr suffix and I don't see we have other code using this convention. PEP 8 calls out we only need a leading _. But I am not going to have a naming convention discussion here :), if you feel it fine, i am fine with it too.
) * Push updates to k8sconfiguration keys and fix issue with known hosts * Remove print statement * Increase CLI version and add to changelog * Remove deprecated CLIError and reduce history.rst text * Joinnis/add validators (Azure#1) * Push updates to k8sconfiguration keys and fix issue with known hosts * Add validations for naming * Remove print statement * Add validator testing to the set of tests * Add unit testing and greater scenario test coverage * Delete test_kubernetesconfiguration_scenario.py * Remove dots from the regex for naming * Add the scenario tests back * Add good key scenario test to scenarios * Remove numeric checks for configurations * Reduce scneario testing * Move validation of configuration name into creation command * Add table formatting for list and show * Update version * Update the error message for validation failure * Update the test cases for the new error messages * Change error message and regex check * Add proper formatting to code files * Updated final formatting checks * Updated error messages * Update error message and help text * Final update to error messaging * Update test_validators.py * Update based on PR comments Co-authored-by: Jonathan Innis <[email protected]>
7 participants
We were loading adal and requests without actually needing any functionality from them in some scenarios.
This PR refactors the Profile class to delay load those particular modules.
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Command Guidelines
(see Authoring Command Modules)