Don't return Suspended accounts when listing by OU

[Anton0]

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:

Resource handler returned message: "Error occurred during operation 'Request REDACTED failed due to: 
AWS SSO is unable to complete your request at this time. 
Obtaining permissions to manage your AWS account 'REDACTED' is taking longer than usual.

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 🤞

[benkehoe]

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.

[stedrow]

+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?

[benkehoe]

Sorry for the delay; this looks mostly good. A couple of changes I'd like:

• Add a parameter exclude_inactive_accts=False to the lookup_accounts_for_ou function, and add it to the status check if statement.
• Move the check to after the caching line.
• Add 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.
• Add yourself to CONTRIBUTORS.md (if you want).

[Anton0]

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).

[benkehoe]

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"?

[Anton0]

That all makes perfect sense to me, thank you! (and updated)