-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Implement v2-debug checks for args, config, customizations, s3, and environment variables #9775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: debug-migration
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## debug-migration #9775 +/- ##
==================================================
Coverage ? 93.28%
==================================================
Files ? 209
Lines ? 16885
Branches ? 0
==================================================
Hits ? 15752
Misses ? 1133
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
return sync_strategies | ||
|
||
def run(self): | ||
def run(self, v2_debug=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this go through self.parameters
instead? That looks like where we're handling other info from parsed_globals
.
encountered_timestamp = False | ||
def identity_with_warning(x): | ||
nonlocal encountered_timestamp | ||
if not encountered_timestamp: | ||
encountered_timestamp = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this flow better in comments? Is it so we're only printing a single warning per response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, without adding these guards I was getting flooded with messages (one per timestamp in the responses), which nearly made the feature unusable.
I'll update with comments
awscli/customizations/paginate.py
Outdated
# This function checks for pagination args in the actual calling | ||
# arguments passed to the function. If the user is using the | ||
# --cli-input-json parameter to provide JSON parameters they are | ||
# all in the API naming space rather than the CLI naming space | ||
# and would be missed by the processing above. This function gets | ||
# called on the calling-command event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally prefer docstrings throughout. Though we don't generate user-facing docs from these, they're more helpful in IDEs for us and contributors.
# The regional us-east-1 endpoint is used in certain cases (e.g. | ||
# FIPS/Dual-Stack is enabled). Rather than duplicating this logic | ||
# from botocore, we check the endpoint URL directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this why we need the actual endpoint check in addition to the us_east_1_regional_endpoint
+ us-east-1
logic above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
awscli/clidriver.py
Outdated
parser = ArgTableArgParser(arg_table) | ||
return parser | ||
|
||
def _detect_migration_breakage(self, session, parsed_args, parsed_globals, arg_table): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only dealing with the file://
, then I think it'd be good to add that to the method name. I wouldn't want someone else to add logic that gets skipped by the early return.
awscli/customizations/globalargs.py
Outdated
_resolve_timeout(session, parsed_args, arg_name) | ||
|
||
def detect_migration_breakage(parsed_args, remaining_args, session, **kwargs): | ||
if parsed_args.v2_debug: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how long this got, it'd be nice to return early here if the mode isn't enabled to save an indent level.
pagination_params_in_input_tokens = [ | ||
param for param in call_parameters if param in input_tokens | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this limited to --cli-input-json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Did some testing and it actually prints the warning even when its specified on the command directly. I'll need to fix this to maybe check what params came from cli-input-json and access them in paginate.py.
Will address in next revision
Description of changes:
--v2-debug
:PYTHONUTF8
andPYTHONIOENCODING
env vars unsupported in v2.aws s3
namespace may call additional APIs for multipart S3 copiesaws cloudformation deploy
will not fail on empty changesets.file://
with blob-type parameters.us-east-1
will use the regional endpoint in v2 rather than global.api_versions
in config will not be supported in v2.[plugins]
will be provisional in v2.--cli-input-json
will disable automatic paging in v2.--v2-debug
code.Description of tests:
PYTHONUTF8
:aws s3 cp
:aws s3 ls
:--cli-input-json
:api_versions
in config file:file://
for a blob-type parameter:After setting
cli_binary_format
toraw-in-base64-out
, I've verified that the warning above was not printed.us-east-1
and executing an S3 command:After setting
region
toaws-global
, I've verified that the warning above was not printed.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.