Closed Anton0 closed 1 year ago
I will take a look at this next week. It makes sense but I want to see if it makes sense to make the filtering configurable.
+1 to this PR, I hit this issue recently. Is this something that can be added in, or is there another solution to this issue already implemented?
Sorry for the delay; this looks mostly good. A couple of changes I'd like:
exclude_inactive_accts=False
to the lookup_accounts_for_ou
function, and add it to the status check if
statement.exclude_inactive_accts=True
to the calls on line 233 of cli/src/aws_sso_util/cfn.py
and line 105 of src/aws_sso_util/cfn_lib/macro.py
.Thank you for the detailed review! I believe this addresses all the requested changes, and I've tested this again in our environment (though only an update operation).
This looks good, one minor style change: you've got if account["Status"] != "ACTIVE" and exclude_inactive_accts is True
. As a rule, I'd put the exclusion check first, because often the thing you're disabling is expensive or may not work correctly (e.g., imagine if accounts didn't always have status or something). It doesn't matter here but I think it makes sense to always do it that way. And then I'd only use an is True
check if I'm expecting that the value can be something other than True
or False
and I want specifically to check for True
instead of truthy. Here's an example I have where the value can be boolean or string, so I check specifically for is True
. Can you change it to if exclude_inactive_accts and account["Status"] != "ACTIVE"
?
That all makes perfect sense to me, thank you! (and updated)
The boto call list_accounts_for_parent includes accounts in with statuses
'ACTIVE'|'SUSPENDED'|'PENDING_CLOSURE'
However assignments cannot be deployed to accounts in the
SUSPENDED
state, trying to results in Cloudformation stalling and eventually returning an error when using the macro:Worth noting the same behavior occurs in the SSO web ui - you are permitted to begin creating the assignment, then it hangs on processing until eventually timing out
I'm not sure if there's a use case for deploying assignments to an account in
PENDING_CLOSURE
nor do I have an account to test that with, but it's easy enough to return those as well if you think it's worth allowing.I'm only using the macro, but I have tested against our accounts and this works as expected.
Resolves #80 hopefully :crossed_fingers: