Do not retrieve users in each goal state #1935
Conversation
narrieta
requested review from
kevinclark19a,
larohra,
pgombar and
ZhidongPeng
as code owners
| @@ -1,53 +0,0 @@ | |||
| # Copyright Microsoft Corporation | |||
There was a problem hiding this comment.
this mock is specialized for the remote access tests so i moved it there
Codecov Report
@@ Coverage Diff @@
## develop #1935 +/- ##
===========================================
- Coverage 69.59% 69.56% -0.03%
===========================================
Files 85 85
Lines 11918 11907 -11
Branches 1670 1673 +3
===========================================
- Hits 8294 8283 -11
+ Misses 3253 3252 -1
- Partials 371 372 +1
Continue to review full report at Codecov.
|
| for data in TestRemoteAccessHandler.eventing_data: | ||
| del data | ||
|
|
||
| # add_user tests | ||
| @patch('azurelinuxagent.common.logger.Logger.info', side_effect=log_info) |
There was a problem hiding this comment.
No point in testing the log statements, removed these checks from the tests
| except RemoteAccessError as rae: | ||
| self.err_message = self.err_message + "Error removing user {0}. Exception: {1}"\ | ||
| .format(user, ustr(rae)) | ||
| self._remove_user(user) |
There was a problem hiding this comment.
Don't we want to keep the log when we are removing the user?
There was a problem hiding this comment.
You are logging it when removing below on L122.
There was a problem hiding this comment.
_remove_user does the logging; 122 is a left-over, will remove it
| raise Exception("test exception for bad username") | ||
| if username in self.all_users: | ||
| raise Exception("test exception, user already exists") | ||
| self.all_users[username] = (username, None, None, None, comment, None, None, expiration) |
There was a problem hiding this comment.
Would a named tuple be better here (line 41)? I'm having trouble understanding what all the "None"s are.
There was a problem hiding this comment.
yes, probably. the existing tests need many improvements. i did several improvements as part of this pr and i may do more if i touch this code in the future.
There was a problem hiding this comment.
(btw i moved this mock from the common utils directory to this specific test, since its functionality is limited to the remote access tests)
| tstuser = "foobar" | ||
| expiration_date = datetime.utcnow() + timedelta(days=1) | ||
| pwd = tstpassword | ||
| rah._add_user(tstuser, pwd, expiration_date) |
There was a problem hiding this comment.
Probably (definitely) out of the scope of this PR, but this should ideally call "run()" instead of a private method (lines 97, 114, 126, etc...). Is it worth capturing this as a future task, or should we just make the changes if these tests break?
There was a problem hiding this comment.
Yes, most of those tests need rewriting and unfortunately that is very common across most of the code base so opening tasks could be unmanageable. I tend to do improvements as I touch related code, many times I span the improvements over several iterations of the code.
There was a problem hiding this comment.
Of course, if a test breaks while changing the prod code we need to address that immediately. When that happens to you, try to evaluate whether the broken test adds any value, on several occasions I've found fixing broken tests is not worth the effort (they may be redundant, test private implementation, etc)
The remote access handler retrieves the list of users on the VM when the goal state doesn't include any jit users (this is done to remove the jit accounts).
Retrieving the users can be extremely slow in some environments and currently it is done an each goal state.
I introduced a flag so that now we retrieve the users only on the first goal state after service start or when the agent has actually created jit accounts.