fix: no BasePath provided to delete base path mapping #13533
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
simonrw
added
semver: patch
simonrw
left a comment
simonrw
left a comment
There was a problem hiding this comment.
Thanks for this submission!
I have commented that we don't think this is the right solution (thanks @bentsku for the correct implementation).
We would also need a validated test showing that this functionality works. You will need to deploy a CFn template (consider using deploy_cfn_template), and take a look at the tests in tests/aws/services/cloudformation/resources/test_apigateway.py for inspiration.
If this is too much let me know and I'll make the changes myself
...ack-core/localstack/services/apigateway/resource_providers/aws_apigateway_basepathmapping.py
Show resolved
Hide resolved
| apigw.delete_base_path_mapping( | ||
| domainName=model["DomainName"], basePath=model.get("BasePath") or "(none)" | ||
| ) |
There was a problem hiding this comment.
issue: I think this is not the right solution. We should be reading the base path from the response to create_base_path_mapping in the create handler, saving it on the model and loading it here. See my suggestion above.
…create_base_path_mapping in the create handler
amitsh1
force-pushed
the
fix/issue_13503
branch
from
309d661 to
cb3a3cc
Compare
simonrw
left a comment
simonrw
left a comment
There was a problem hiding this comment.
Hi, thanks for updating this PR. Unfortunately we cannot accept the test in its current form. See the comments for specifics.
There was a problem hiding this comment.
issue: thanks for adding this test, but without showing that this test is validated, we don't know if the behaviour is correct.
We try to validate our tests against AWS, meaning that with a change of (external) configuration a test will run against AWS and LocalStack with identical behaviour. You can see more information here. Unfortunately without doing this we cannot accept this test.
If you wish to continue with this PR, please follow our testing practices. Note: this requires access to a real AWS account.
Otherwise I am happy to take over and finish this PR.
|
|
||
|
|
||
| class TestApiGatewayBasePathMapping: | ||
| def test_delete_base_path_mapping_missing_base_path( |
There was a problem hiding this comment.
issue: you have not added a validation classification marker. Our test suite does not allow tests to be run without one of these markers, so this suggests you have not run this test.
| def test_delete_base_path_mapping_missing_base_path( | |
| @markers.aws.validated | |
| def test_delete_base_path_mapping_missing_base_path( |
There was a problem hiding this comment.
issue: this test should be moved to tests/aws/services/cloudformation/resources/test_apigateway.py since it is specifically a test for the API Gateway CloudFormation resources.
| "CertificateArn" | ||
| ] | ||
|
|
||
| template = json.dumps( |
There was a problem hiding this comment.
issue: we prefer putting our CloudFormation templates into a file under tests/aws/templates and referring to them in the test itself. This makes the test body much smaller, and the CloudFormation template more understandable. Also could you use yaml instead of JSON - it's a bit nicer to read IMO
3 participants
Motivation
closes #13503
Changes
BasePath was not being provided by cloudformation on deletion because it is not being saved the base path mapping object
Tests
Related
Resolves UNC-174