Include OrganizationId in each CoveSessionInformation about botocove HOT 9 CLOSED

Sure - sounds helpful! Should be simple enough to just add a line below and update the typed dict if you want to put a PR up?

I can't think of any reason not to just slice out the org ID from the ARN: maybe run the official regex against it and raise on malfored/catch a value error just in case - bit overkill for a code path we'll never really hit but can't hurt.

from botocove.

The official regex appears in the Account data type documentation and looks like this:

^arn:aws:organizations::\d{12}:account\/o-[a-z0-9]{10,32}\/\d{12}

Although I've never seen one with more than 10, officially an organization ID can have up to 32 digits!

That's a good catch. I'll try using a regex in my PR and test with different lengths of organization IDs.

from botocove.

I'll have a go at this one after we merge #36 .

from botocove.

Hint taken :) We can get that merged tomorrow - some quick feedback/q's

from botocove.

Thanks! I'll be out for the rest of this week so there's no rush. Hope you're feeling better!

from botocove.

TL;DR: I think I should refactor the tests some more before adding this feature! What do you think about the proposed feature-oriented testing I describe below?

Before I even had a serious go at implementing this I did the most basic thing and it opened another can of worms in the unit tests 😃

Adding a new key to the CoveSessionInformation dict fails some tests that check the shape of the output result.

I hard-coded an OrganizationId key in my add_organization_id branch so you can see how it affects the tests.

The full CI output is really verbose. You can use this command instead to print just the names of the tests that fail.

poetry run pytest \
--verbosity=1 \
--show-capture=no \
--tb=no \
--no-summary \
--no-header \
| awk '/FAILED/ {print $1}'

These are the tests that fail:

tests/test_decorator.py::test_decorated_simple_func_passed_args
tests/test_decorator_no_args.py::test_decorated_simple_func
tests/test_decorator_no_args.py::test_decorated_func_passed_arg
tests/test_decorator_no_args.py::test_decorated_func_passed_arg_and_kwarg
tests/test_exceptions.py::test_handled_exception_in_wrapped_func
tests/test_session.py::test_session_result_formatter
tests/test_session.py::test_session_result_formatter_with_policy
tests/test_session.py::test_session_result_formatter_with_policy_arn

They fail because they expect an exact output shape from the cove function.

The easy way out would be to modify these tests to expect an OrganizationId key from now on. But that seems like it's going to make maintenance more difficult. I think a better long term solution is to make the tests less brittle.

It would be easier to add and remove features of the output if each test made less assumptions about the overall shape of the result. By orienting the tests around features instead of exact return values of the cove function I think we can improve the maintainability.

For example, the tests for the OrganizationId should check only the presence of an OrganizationId key and the content and formatting of its value.

But what should we do about the existing tests that fail?

Take as an example the assertion in tests/test_session.py::test_session_result_formatter:

def test_session_result_formatter(org_accounts: List[AccountTypeDef]) -> None:
    @cove
    def simple_func(session: CoveSession, a_string: str) -> str:
        return a_string

    # Only one account for simplicity
    cove_output = simple_func("test-string")
    expected = [
        {
            "Id": org_accounts[1]["Id"],
            "Arn": org_accounts[1]["Arn"],
            "Email": org_accounts[1]["Email"],
            "Name": org_accounts[1]["Name"],
            "Status": "ACTIVE",
            "AssumeRoleSuccess": True,
            "Result": "test-string",
            "RoleName": "OrganizationAccountAccessRole",
            "RoleSessionName": "OrganizationAccountAccessRole",
        }
    ]
    assert cove_output["Results"] == expected

It's unclear exactly what this test is for. It seems that each key ought to be covered already by feature-oriented tests for the Result, the RoleName, the RoleSession name, and so on. My first thought is that this test should just be deleted.

Compare that assertion to the next test in the file, tests/test_session.py::test_session_result_formatter_with_policy:

def test_session_result_formatter_with_policy(
    org_accounts: List[AccountTypeDef],
) -> None:
    session_policy = '{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Action":"*","Resource":"*"}]}'  # noqa: E501

    @cove(policy=session_policy)
    def simple_func(session: CoveSession, a_string: str) -> str:
        return a_string

    # Only one account for simplicity
    cove_output = simple_func("test-string")
    expected = [
        {
            "Id": org_accounts[1]["Id"],
            "Arn": org_accounts[1]["Arn"],
            "Email": org_accounts[1]["Email"],
            "Name": org_accounts[1]["Name"],
            "Status": "ACTIVE",
            "AssumeRoleSuccess": True,
            "Result": "test-string",
            "RoleName": "OrganizationAccountAccessRole",
            "RoleSessionName": "OrganizationAccountAccessRole",
            "Policy": session_policy,
        }
    ]
    assert cove_output["Results"] == expected

The name of the test suggests that we are testing for the session policy feature. It checks that the Policy key exists but checks also for a bunch of unrelated keys. We could make this test more descriptive and less brittle by renaming it and simplifying the assertion.

Name: test_when_cove_has_policy_then_session_info_has_policy_item

Assertion:

expected_item = ("Policy", session_policy)
result_items = cove_output["Results"][0].items()
assert expected_item in actual_items

And now, thinking about the first example again in terms of features, it looks like what you might expect to happen when no session policy is used. So maybe instead of just deleting it we can replace it with a feature-oriented test:

Name: test_when_cove_has_no_policy_then_session_info_has_no_policy_key

Assertion:

expected_key = "Policy"
actual_keys = cove_output["Results"][0].keys()
assert expected_key not in actual_keys

It might still be valuable to have an integration test (if that's the right word for this sort of thing) for the expected overall shape for a range of input configurations. Right now I'm unsure how that would look.

There are certain keys that we want to be there always. For example every CoveSessionInformation is going to have at least the Id key. You could say that it's an overall postcondition of the cove function. I dislike the idea of duplicating a check for the Id in every feature-oriented test, but I would like the test suite to do the check for us so that we can detect when a feature does something weird like delete a key that should always be there.

I don't know if it's the generally accepted way of doing it (I didn't find an example in the PyTest documentation!) but I found a way to implement general postconditions. For a working example you can check my experiments in test_build_org_tree.py.

By grouping the tests for each scenario into a test class and defining one or more postcondition classes from which each scenario class would inherit, you can "mix in" postcondition checks to each scenario without repeating yourself.

The trick is that the postcondition class must not be named Test* but must have methods named test_*. Each scenario class must be named Test* to be executed at all and must inherit from ResultPosconditions to include the result postcondition checks.

from botocove.

I see where you're coming from, but not sure this is really worth worrying about - I'd rather not break backwards compatability by removing output, I can't really imagine we'll add much more functionality. I was just going to delete these tests in a post-moto world but you kindly fixed them up :D Some of them, as you've noticed, are just artifacts of early dev where a "does this actually work" was the purpose rather than any regression or feature/functionality.

Adding complexity in the test suite isn't something I'm particularly inclined to do - just keeping one test that asserts mandatory fields are all present is probably enough :)

from botocove.

I'd rather not break backwards compatability by removing output

I don't want to remove anything from the real output either just now, because it would break backward compatibility as you say. There some fields that I tend to ignore because they are always the same for the whole session (Policy, RoleSessionName for example). But it's easy enough for me to just select the keys I care about in post-processing rather than making risky changes to botocove! Someone somewhere might be using them since they are available, so it's better just to leave them there.

Adding complexity in the test suite isn't something I'm particularly inclined to do - just keeping one test that asserts mandatory fields are all present is probably enough :)

That's quite sensible. The test code should be simple. Even if it would work in practice what I described about using inheritance to implement preconditions, I don't think its really worth the effort either just now.

Do you still see value in a test of the style "when Policy is passed then assert the Policy key is in the output" and "when Policy is not passed then assert that the Policy key is not in the output"? I would be willing to do that refactoring. I think the tests would still be simple and when spelled out like that it would also serve as an executable spec.

from botocove.

I no longer believe an OrganizationId key needs to be part of botocove's output.

I think it's better to add minimal frills and stick to doing one thing well: the iteration over accounts and the collection of results.

The opening message shows a simple workaround wherever the organization ID is required. I don't mind including that in all my scripts.

from botocove.