Skip to content
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

[boto] Default to None if no region #526

Merged
merged 4 commits into from
Aug 6, 2018
Merged

Conversation

Kyle-Verhoog
Copy link
Member

Fixes #525, also applies formatting.

@Kyle-Verhoog Kyle-Verhoog added this to the 0.13.0 milestone Aug 1, 2018
@Kyle-Verhoog Kyle-Verhoog changed the base branch from 0.13-dev to master August 1, 2018 15:18
@Kyle-Verhoog Kyle-Verhoog assigned Kyle-Verhoog and unassigned SeanOC Aug 1, 2018
Copy link

@palazzem palazzem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change in the meta list. Do we have a way to introduce a test to avoid a regression?

'aws.region': region_name,
"aws.agent": "boto",
"aws.operation": operation_name,
"aws.region": region_name,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the None default, get_region_name(region) returns None. Maybe we should add aws.region tag only if it's not none?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer this behaviour, but I opted to stay consistent with the functionality of line 143:

region = getattr(instance, "region", None)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will result in the region tag showing up as 'None', which isn't ideal. Let's not add it if it's none.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly, let's not add it if it's None

@Kyle-Verhoog
Copy link
Member Author

Giving it a quick look, the moto library we use to mock boto doesn't seem to allow us to mock the region not being set. Creating a regression test would require a non-trivial amount of work.

}
if region_name:
meta["aws.region"] = region_name
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move these as a constants? better in a constants.py module. So they're all in one place + you use the same variable in 2 parts of the code

Copy link

@SeanOC SeanOC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Probably would be good to queue a follow-up task to make an upstream patch to moto to allow region to be empty and then provide a test here.

@Kyle-Verhoog Kyle-Verhoog merged commit 90ba860 into master Aug 6, 2018
@Kyle-Verhoog Kyle-Verhoog deleted the kyle-verhoog/525 branch August 6, 2018 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants