-
Notifications
You must be signed in to change notification settings - Fork 29
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
aws: add default regions in aws-china and aws-us-gov #87
base: master
Are you sure you want to change the base?
Conversation
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.
Sorry for the delayed response, was bzy with project work in Q1.
Needs changed to your code.
default_regions = ['us-west-2', 'cn-northwest-1', 'us-gov-west-1'] | ||
for region in default_regions: | ||
try: | ||
with compute_client("aws", aws_region=region) as client: | ||
regions = client.list_regions() | ||
if regions: | ||
break | ||
except Exception as e: | ||
pass | ||
if "all" in regions: | ||
logger.error(f"Unable to retrive region list using currrent token!") | ||
sys.exit(1) |
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.
default_regions = ['us-west-2', 'cn-northwest-1', 'us-gov-west-1'] | |
for region in default_regions: | |
try: | |
with compute_client("aws", aws_region=region) as client: | |
regions = client.list_regions() | |
if regions: | |
break | |
except Exception as e: | |
pass | |
if "all" in regions: | |
logger.error(f"Unable to retrive region list using currrent token!") | |
sys.exit(1) | |
default_regions = ['us-west-2', 'cn-northwest-1', 'us-gov-west-1'] | |
regions = [] | |
for region in default_regions: | |
try: | |
with compute_client("aws", aws_region=region) as client: | |
_regions = client.list_regions() | |
if _regions: | |
regions += _regions | |
break | |
except Exception as e: | |
pass | |
else: | |
logger.error(f"Unable to retrive region list using currrent token!") | |
sys.exit(1) |
You should append the list of regions from all default regions to iterate over.
Also, the error should only be thrown when break
is not hit in any cycle of the loop. That is else
.
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.
You should append the list of regions from all default regions to iterate over.
That's not what the proposed patch does, afaics, because of the break
.
Does .list_regions()
return all the regions, or just some?
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.
The .list_regions() can only retrieve the regions your account belongs. That means the default region lists(['us-west-2', 'cn-northwest-1', 'us-gov-west-1']) is isolated physically and managed by standalone IAM manage systems.
Ping @liangxiao1 ! |
|
Handle failures When pass "all" in regions, us-west-2 is not proper when token is for aws-china and aws-us-gov. Signed-off-by: Xiao Liang <xiliang@redhat.com>
@liangxiao1 Let me know once you push the code as per my comments! Also pre-commit checks are failing , please look into that! |
Handle failures When pass "all" in regions, us-west-2 is not proper when token is for aws-china and aws-us-gov.
Signed-off-by: Xiao Liang xiliang@redhat.com