Code freeze base branch #96
Conversation
gpalmz
requested review from
benblaustein and
benjjs
and removed request for
benblaustein
gpalmz
changed the title
| def execute_json_nullable(az_cmd: AzCmd) -> Any: | ||
| az_response = execute(az_cmd, can_fail=True) | ||
| if not az_response: | ||
| return None | ||
| return json.loads(az_response) |
There was a problem hiding this comment.
Hm, I feel like this was supposed to be used somewhere.... If we find it I think it would be better off as an optional argument to execute_json.
| def get_assigned_entra_role_ids(user_id: str) -> set[str]: | ||
| return set( | ||
| execute_json( | ||
| Cmd(["az", "rest"]) |
There was a problem hiding this comment.
Are these not using AzCmd because there isn't a service associated with these commands? Seems a little bit confusing just from a naming perspective given that they are very much az cli commands.
There was a problem hiding this comment.
Right. I kinda think we should phase out that AzCmd class, I don't really see the benefit of it. I also think we could consider integrating the shlex.quote into the Cmd class to make it easier to use. Anything that's an input to a command should proably be quoted (I think, lmk if you think that's not right)
There was a problem hiding this comment.
Yeah I think we should quote everything - just have to be very careful to not double quote things. The PR for that change will need to be tested very thoroughly.
benjjs
left a comment
benjjs
left a comment
There was a problem hiding this comment.
Approach seems good and the refactor is otherwise appreciated
…rest [AZINTS-4252] Use `az rest` to fetch permissions
gpalmz
changed the title
[AZINTS-4268][HD-148] Quickstart agent bulk install for Azure VMs
[AZINTS-4260] Handle unhandled throttling
…signments-concurrent [AZINTS-4143] Check ability to create app registrations (concurrently)
Will use this as base branch until code thaw.