integ-tests: clean up code for IAM policies, IAM roles, KMS keys creation #2325
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2325 +/- ##
===========================================
+ Coverage 61.77% 61.83% +0.05%
===========================================
Files 40 40
Lines 6180 6186 +6
===========================================
+ Hits 3818 3825 +7
+ Misses 2362 2361 -1
Continue to review full report at Codecov.
|
hanwen-pcluste
added
the
skip-changelog-update
hanwen-pcluste
force-pushed
the
develop
branch
3 times, most recently
from
c0f9c49 to
48b84ee
Compare
chenwany
left a comment
chenwany
left a comment
There was a problem hiding this comment.
Have been test_ebs_single be tested with different schedulers?
tests/integration-tests/utils.py
Outdated
| assert_that(target["IpRanges"][0]["CidrIp"]).is_equal_to(expected_cidr) | ||
|
|
||
|
|
||
| def get_arn_partition(region): |
There was a problem hiding this comment.
There is a function in our code, get_partition(), can we simplify our code here like that?
def get_partition():
"""Get partition for the AWS_DEFAULT_REGION set in the environment."""
region = get_region()
return next(("aws-" + partition for partition in ["us-gov", "cn"] if region.startswith(partition)), "aws")
| user_iam_role = self.role_factory( | ||
| region, "ec2", [policies["awsbatch_instance_policy"], policies["traditional_instance_policy"]] | ||
| ) | ||
| self.kms_key_id = self._create_kms_key(region, user_iam_role) |
There was a problem hiding this comment.
You define self.kms_key_id here, why removing self.kms_key_id = None in __init__?
| region, "ec2", [policies["awsbatch_instance_policy"], policies["traditional_instance_policy"]] | ||
| ) | ||
| self.kms_key_id = self._create_kms_key(region, user_iam_role) | ||
| self.key_and_role_by_region[region] = (self.kms_key_id, user_iam_role) |
There was a problem hiding this comment.
minor: it will be more clear for me to understand it is the combination of key id and role name if we name it self.key_id_and_role_by_region, since we have lots of role arn, role name, key name, key id here
| env = Environment(loader=file_loader, trim_blocks=True, lstrip_blocks=True) | ||
| key_policy = env.get_template("key_policy.json").render( | ||
| partition=self.partition, account_id=self.account_id, iam_role_name=self.iam_role | ||
| partition=partition, account_id=account_id, iam_role_name=user_iam_role |
There was a problem hiding this comment.
minor: we can write here as partition=get_arn_partition(region) and remove line 64
tests/integration-tests/conftest.py
Outdated
| iam_client.detach_role_policy(RoleName=role_name, PolicyArn=policy) | ||
| logging.info(f"Deleting iam role {role_name}") | ||
| iam_client.delete_role(RoleName=role_name) | ||
| time.sleep(60) |
There was a problem hiding this comment.
Could you add a explanation for the time.sleep here?
hanwen-pcluste
force-pushed
the
develop
branch
from
48b84ee to
f08638a
Compare
2 participants
This PR will complete the leftover from #2304. In that PR, we duplicated code for IAM policies, IAM roles creation. We didn't clean up the duplication to limit the scale of that PR.
Moreover, this PR will scope the fixtures for IAM policies, IAM roles, KMS keys to session, allowing reusing of the resources across tests. Note that the resources are not reused across parallel test runs.
This PR is verified by the following integration tests:
Signed-off-by: Hanwen hanwenli@amazon.com
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.