Add region to CLI arguments#16
Conversation
anasstahr
requested review from
anasstahr
and removed request for
anasstahr
acmlau
force-pushed
the
update-cli-arguments
branch
from
8840e8f to
3058d7a
Compare
|
Even though the default was change, there are tests that didn't reflect the change (and are somehow still passing?) |
|
Should the documentation also be changed to reflect the change in defaults? https://github.com/aws/aws-mcp-proxy/blob/0bf5c98ba14bf1021995a9905d09a438525fa290/tests/test_sigv4_helper.py#L246 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
acmlau
force-pushed
the
update-cli-arguments
branch
from
1a3be58 to
273a37e
Compare
|
|
||
| # Get region from parameter, environment variable, or default | ||
| if not region: | ||
| region = os.environ.get('AWS_REGION', 'us-west-2') |
There was a problem hiding this comment.
We should pass the region here, from the parsed args rather than fetching it again.
There was a problem hiding this comment.
As a standalone function, it makes sense to check whether region is not set.
I did notice that we never passed the region as a parameter when creating the sigv4_auth https://github.com/aws/aws-mcp-proxy/blob/main/aws_mcp_proxy/server.py#L60, will create new PR to address that + removing some unused parameters
There was a problem hiding this comment.
By the time we create sigv4 auth, region should have been resolved, and region should not be None
|
There should not be a default region, region should come from the following order
|
acmlau
force-pushed
the
update-cli-arguments
branch
from
077a66f to
1b1b447
Compare
acmlau
force-pushed
the
update-cli-arguments
branch
from
1b1b447 to
075a918
Compare
acmlau
force-pushed
the
update-cli-arguments
branch
from
cfac71f to
89debe0
Compare
|
|
||
| # Get region from parameter, environment variable, or default | ||
| if not region: | ||
| region = os.environ.get('AWS_REGION', 'us-west-2') |
There was a problem hiding this comment.
By the time we create sigv4 auth, region should have been resolved, and region should not be None
There was a problem hiding this comment.
| *, service: str, region: str, profile: Optional[str] = None, |
Let's see if we can make region a required argument. create_sigv4_auth expect the region to be resolved lower in the call stack somewhere.
Also adding *, because the function take three str arguments, it is easy to mess up during dev.
There was a problem hiding this comment.
Yeah, ideally Region etc should be done during parse_args (or some new parent function like resolve_args that merges cli args with env args and defaults into a dataclass we can pass around)
| url: str, | ||
| service: str, | ||
| profile: Optional[str] = None, | ||
| region: Optional[str] = None, |
There was a problem hiding this comment.
region is not optional at this point.
acmlau
force-pushed
the
update-cli-arguments
branch
from
21a6e41 to
f0e2a59
Compare
acmlau
force-pushed
the
update-cli-arguments
branch
from
f0e2a59 to
a916495
Compare
acmlau
force-pushed
the
update-cli-arguments
branch
from
fe99c9d to
82a3267
Compare
|
|
||
| result = determine_aws_region(endpoint, region) | ||
|
|
||
| assert result == region |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| result = determine_aws_region(endpoint) | ||
|
|
||
| assert result == expected_region |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| result = determine_aws_region(endpoint) | ||
|
|
||
| assert result == expected_region |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| with pytest.raises(ValueError) as exc_info: | ||
| determine_aws_region(endpoint) | ||
|
|
||
| assert 'Could not determine AWS region' in str(exc_info.value) |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| determine_aws_region(endpoint) | ||
|
|
||
| assert 'Could not determine AWS region' in str(exc_info.value) | ||
| assert endpoint in str(exc_info.value) |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert 'Could not determine AWS region' in str(exc_info.value) | ||
| assert endpoint in str(exc_info.value) | ||
| assert '--region argument' in str(exc_info.value) |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
acmlau
force-pushed
the
update-cli-arguments
branch
from
82a3267 to
6bb0c5f
Compare
| assert isinstance(result, SigV4HTTPXAuth) | ||
| assert result.service == 'test-service' | ||
| assert result.region == 'eu-west-1' # from environment | ||
| assert result.region == 'test-region' # default region |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
6 participants
Description of changes:
--regionargument to specify region from command lineBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.