Conversation
sbiscigl
left a comment
sbiscigl
left a comment
There was a problem hiding this comment.
Think we might want to add a test to the existing s3 control tests so that we test for this and can prevent it from coming back.
| #if(!$operation.request.shape.hasEventStreamMembers() && !($serviceNamespace == "S3Crt" && $operation.s3CrtEnabled)) | ||
| ${indent} AWS_OPERATION_CHECK_SUCCESS(endpointResolutionOutcome, ${operation.name}, CoreErrors, CoreErrors::ENDPOINT_RESOLUTION_FAILURE, endpointResolutionOutcome.GetError().GetMessage()); | ||
| #if($operation.hasEndpointTrait)## Note: EndpointDiscovery Trait is not Endpoint Trait | ||
| #if($serviceNamespace == "S3Control" && $serviceModel.rawEndpointRules) |
There was a problem hiding this comment.
This seems like a messy if statement
if($operation.hasEndpointTrait) {
if ($serviceNamespace == "S3Control" && $serviceModel.rawEndpointRules) {
// do nothing
} else {
addPrefix()
}
}
something better would be
if(operation.hasEndpointTrait && $serviceNamespace != "S3Control" && !$serviceModel.rawEndpointRules) {
addPrefix()
}
additionally why not just simply
if(operation.hasEndpointTrait && !$serviceModel.rawEndpointRules) {
addPrefix()
}
since we will always be providing a endpoint rules set in our default generation path?
There was a problem hiding this comment.
For now will just limit to s3control for the scope of the problem unless really required
There was a problem hiding this comment.
The suggestions are not quite right.
The statement right now allows all add prefix for all cases except (s3control && endpoint2.0)
The simplified negation of this is actually !s3control || !endpoint2.0 per De Morgan's law . So i can simplify to that.
https://en.wikipedia.org/wiki/De_Morgan%27s_laws
sbera87
force-pushed
the
prefix
branch
2 times, most recently
from
ac2cb11 to
c4ff985
Compare
| .WithBucket(bucket) | ||
| .WithAccountId(m_accountId) | ||
| .WithBucketAccountId(m_accountId); | ||
| auto resolveEndpointOutcome = s3controlClient.accessEndpointProvider()->ResolveEndpoint(request.GetEndpointContextParams()); |
There was a problem hiding this comment.
this doesnt actually test the change you made, it tests the endpoint provider, which worked correctly before. You're actually changing client source, so a adequate test would be calling s3controlClient.CreateAccessPoint
| #if(!$operation.request.shape.hasEventStreamMembers() && !($serviceNamespace == "S3Crt" && $operation.s3CrtEnabled)) | ||
| ${indent} AWS_OPERATION_CHECK_SUCCESS(endpointResolutionOutcome, ${operation.name}, CoreErrors, CoreErrors::ENDPOINT_RESOLUTION_FAILURE, endpointResolutionOutcome.GetError().GetMessage()); | ||
| #if($operation.hasEndpointTrait)## Note: EndpointDiscovery Trait is not Endpoint Trait | ||
| #if(!($serviceNamespace == "S3Control" && $serviceModel.rawEndpointRules)) |
There was a problem hiding this comment.
please collapse the if statement to a single if statement, nested if statements should be avoided when possible.
if($operation.hasEndpointTrait && !($serviceNamespace == "S3Control" && $serviceModel.rawEndpointRules))
| const auto requestSeen = mock_http_client->GetMostRecentHttpRequest(); | ||
| EXPECT_EQ("https://s3express-control.us-east-1.amazonaws.com/v20180820/accesspoint/test-ap--use2-az1--xa-s3", requestSeen.GetUri().GetURIString()); | ||
|
|
||
| mock_client_factory.reset(); |
There was a problem hiding this comment.
nit -- dont need to call reset on a shared_ptr, RAII will do that automatically
2 participants
Issue #, if available:
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.