Fix bootstrap tests failing in non-default region by viren-nadkarni · Pull Request #10152 · localstack/localstack

viren-nadkarni · 2024-01-31T13:54:58Z

Summary

This PR fixes the following bootstrap test when run in non-default account ID/region:

tests.bootstrap.test_cosmetic_configuration.TestLocalStackHost.test_scenario

Changes

The S3 bucket had the account ID and region hardcoded in the name within the CDK template. When the tests were run in a different account ID or region, the bucket name would be different causing the test to fail.

Tests

See ci/circleci: bootstrap-tests job for commit 707ebf9 🟢 - this run used non-default test credentials to demonstrate that the tests pass

github-actions · 2024-01-31T14:01:33Z

S3 Image Test Results (AMD64 / ARM64)

2 files 2 suites 3m 8s ⏱️
386 tests 335 ✅ 51 💤 0 ❌
772 runs 670 ✅ 102 💤 0 ❌

Results for commit 707ebf9.

♻️ This comment has been updated with latest results.

TO BE REVERTED BEFORE MERGE

github-actions · 2024-01-31T14:50:56Z

LocalStack Community integration with Pro

2 files 2 suites 1h 22m 38s ⏱️
2 618 tests 2 370 ✅ 248 💤 0 ❌
2 620 runs 2 370 ✅ 250 💤 0 ❌

Results for commit b3686f1.

♻️ This comment has been updated with latest results.

This reverts commit 0d2171c.

viren-nadkarni · 2024-02-01T10:37:56Z

tests/bootstrap/test_cosmetic_configuration.py

            infra.add_custom_setup(
                lambda: load_python_lambda_to_s3(
                    s3_client=aws_client.s3,
-                    bucket_name=infra.get_asset_bucket(),


localstack/localstack/testing/scenario/provisioning.py

Lines 102 to 105 in b3686f1

def get_asset_bucket(self):

account = self.aws_client.sts.get_caller_identity()["Account"]

region = self.aws_client.sts.meta.region_name

return f"localstack-testing-{account}-{region}"

I'm not certain about the intended purpose of get_asset_bucket() helper so I've not changed its behaviour.

I think this makes sense in this case, but we probably want to allow infrastructure_setup to take account/region. Then this helper would be OK I think? cc @dominikschubert

See

localstack/tests/bootstrap/conftest.py

Lines 22 to 37 in 896ba4b

def _infrastructure_setup(

namespace: str, force_synth: Optional[bool] = False, port: int = constants.DEFAULT_PORT_EDGE

) -> InfraProvisioner:

"""

:param namespace: repo-unique identifier for this CDK app.

A directory with this name will be created at `tests/aws/cdk_templates/<namespace>/`

:param force_synth: set to True to always re-synth the CDK app

:return: an instantiated CDK InfraProvisioner which can be used to deploy a CDK app

"""

return InfraProvisioner(

base_path=cdk_template_path,

aws_client=aws_client_factory(endpoint_url=f"http://localhost:{port}"),

namespace=namespace,

force_synth=force_synth,

persist_output=True,

)

We build a new client factory here without accepting region/account configuration.

I'll make a note of it, since I'll soon try to rework the provisioner a bit for asset support as well 👍 For now the solution seems fine, the bucket naming was mostly done for compatibility with live-AWS which isn't really relevant for bootstrap tests here

simonrw

Thanks for tackling this issue. I agree this should be fixed, but I am unsure if the change you have made is correct. I have approved since it works, but this might need to be upstreamed into the CDK setup code to cover more cases. What do you think @dominikschubert?

simonrw · 2024-02-01T18:05:55Z

tests/bootstrap/test_cosmetic_configuration.py

            infra.add_custom_setup(
                lambda: load_python_lambda_to_s3(
                    s3_client=aws_client.s3,
-                    bucket_name=infra.get_asset_bucket(),


I think this makes sense in this case, but we probably want to allow infrastructure_setup to take account/region. Then this helper would be OK I think? cc @dominikschubert

See

localstack/tests/bootstrap/conftest.py

Lines 22 to 37 in 896ba4b

def _infrastructure_setup(

namespace: str, force_synth: Optional[bool] = False, port: int = constants.DEFAULT_PORT_EDGE

) -> InfraProvisioner:

"""

:param namespace: repo-unique identifier for this CDK app.

A directory with this name will be created at `tests/aws/cdk_templates/<namespace>/`

:param force_synth: set to True to always re-synth the CDK app

:return: an instantiated CDK InfraProvisioner which can be used to deploy a CDK app

"""

return InfraProvisioner(

base_path=cdk_template_path,

aws_client=aws_client_factory(endpoint_url=f"http://localhost:{port}"),

namespace=namespace,

force_synth=force_synth,

persist_output=True,

)

We build a new client factory here without accepting region/account configuration.

dominikschubert

LGTM

dominikschubert · 2024-02-05T12:47:01Z

tests/bootstrap/test_cosmetic_configuration.py

            infra.add_custom_setup(
                lambda: load_python_lambda_to_s3(
                    s3_client=aws_client.s3,
-                    bucket_name=infra.get_asset_bucket(),


I'll make a note of it, since I'll soon try to rework the provisioner a bit for asset support as well 👍 For now the solution seems fine, the bucket naming was mostly done for compatibility with live-AWS which isn't really relevant for bootstrap tests here

Fix bootstrap tests failing in non-default region

25f148c

viren-nadkarni self-assigned this Jan 31, 2024

viren-nadkarni added the semver: patch Non-breaking changes which can be included in patch releases label Jan 31, 2024

[DEBUG] Use non default account and region

0d2171c

TO BE REVERTED BEFORE MERGE

viren-nadkarni force-pushed the bootstrap-tests-xaccount branch from 818483b to b34b0c4 Compare February 1, 2024 06:46

Fix creds

3b4bab4

viren-nadkarni force-pushed the bootstrap-tests-xaccount branch from b34b0c4 to 3b4bab4 Compare February 1, 2024 06:57

viren-nadkarni added 3 commits February 1, 2024 13:56

Remove use of dynamic asset bucket name

707ebf9

Revert "[DEBUG] Use non default account and region"

1f43bd3

This reverts commit 0d2171c.

Revert use of creds

b3686f1

viren-nadkarni commented Feb 1, 2024

View reviewed changes

viren-nadkarni marked this pull request as ready for review February 1, 2024 10:46

viren-nadkarni requested a review from simonrw as a code owner February 1, 2024 10:46

viren-nadkarni requested review from dfangl and joe4dev February 1, 2024 10:46

simonrw approved these changes Feb 1, 2024

View reviewed changes

viren-nadkarni requested a review from dominikschubert February 2, 2024 05:08

dominikschubert approved these changes Feb 5, 2024

View reviewed changes

viren-nadkarni merged commit 75042c5 into master Feb 5, 2024

viren-nadkarni deleted the bootstrap-tests-xaccount branch February 5, 2024 13:20

maxhoheiser pushed a commit that referenced this pull request Feb 5, 2024

Fix bootstrap tests failing in non-default region (#10152)

ad8fdc2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix bootstrap tests failing in non-default region#10152

Fix bootstrap tests failing in non-default region#10152
viren-nadkarni merged 6 commits intomasterfrom
bootstrap-tests-xaccount

viren-nadkarni commented Jan 31, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jan 31, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jan 31, 2024 •

edited

Loading

Uh oh!

viren-nadkarni Feb 1, 2024 •

edited

Loading

Uh oh!

simonrw Feb 1, 2024

Uh oh!

dominikschubert Feb 5, 2024

Uh oh!

simonrw left a comment

Uh oh!

simonrw Feb 1, 2024

Uh oh!

dominikschubert left a comment

Uh oh!

dominikschubert Feb 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	def get_asset_bucket(self):
	account = self.aws_client.sts.get_caller_identity()["Account"]
	region = self.aws_client.sts.meta.region_name
	return f"localstack-testing-{account}-{region}"

	def _infrastructure_setup(
	namespace: str, force_synth: Optional[bool] = False, port: int = constants.DEFAULT_PORT_EDGE
	) -> InfraProvisioner:
	"""
	:param namespace: repo-unique identifier for this CDK app.
	A directory with this name will be created at `tests/aws/cdk_templates/<namespace>/`
	:param force_synth: set to True to always re-synth the CDK app
	:return: an instantiated CDK InfraProvisioner which can be used to deploy a CDK app
	"""
	return InfraProvisioner(
	base_path=cdk_template_path,
	aws_client=aws_client_factory(endpoint_url=f"http://localhost:{port}"),
	namespace=namespace,
	force_synth=force_synth,
	persist_output=True,
	)

Uh oh!

Conversation

viren-nadkarni commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Tests

Uh oh!

github-actions bot commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

S3 Image Test Results (AMD64 / ARM64)

Uh oh!

github-actions bot commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

viren-nadkarni Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonrw Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

dominikschubert Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

simonrw left a comment

Choose a reason for hiding this comment

Uh oh!

simonrw Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

dominikschubert left a comment

Choose a reason for hiding this comment

Uh oh!

dominikschubert Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

viren-nadkarni commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

viren-nadkarni Feb 1, 2024 •

edited

Loading