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.

Sign up for GitHub

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

Jump to bottom

Add an RDS offer #9

Merged
merged 5 commits into from
May 22, 2017
Merged

Add an RDS offer #9

merged 5 commits into from
May 22, 2017

Conversation

bclodius
Copy link
Contributor

A few notes about this PR

  • database_edition is only valid for Oracle and SQL Server, without this parameter I was facing hash collisions
  • license_model has different values based on the database_engine and sometimes multiple options for a database_engine; so made it required.

@garrettheel garrettheel reopened this May 21, 2017
garrettheel
garrettheel reviewed May 21, 2017
View reviewed changes
Copy link
Contributor

@garrettheel garrettheel left a comment

Choose a reason for hiding this comment

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

Looks great! The only small thing that'd be nice to see is something testing the reserved_hourly and reserved_upfront methods.

It looks like there was a hiccup between on the Github webhook so Travis didn't build this PR. The error was GitHub payload is missing a merge commit (mergeable_state: "unknown", merged: false). I closed and re-opened to kick things off again - do you wanna have a look at the mypy errors in the py3.6 build?

Can you also try the CLA again when you get a chance and let me know if it's working now? If not please send me some more details about what you see and the errors and I'll take a closer look.

Thanks!

@garrettheel garrettheel changed the title Adds RDS offers; Fixes #7 Adds an RDS offer May 21, 2017
@garrettheel garrettheel changed the title Adds an RDS offer Add an RDS offer May 21, 2017
@bclodius
Copy link
Contributor Author

@garrettheel

  1. I'll certainly add some test cases for reserved_hourly and reserved_upfront

  2. I'll review the errors for mypy for 3.6 and fix

  3. The signing of CLA worked on retry

bclodius
bclodius commented May 22, 2017
View reviewed changes
awspricing/offers.py
@@ -114,7 +117,8 @@ def _generate_reverse_sku_mapping(self,
if (product_family is not None and
product['productFamily'] != product_family):
continue
attrs = [product['attributes'][attr] for attr in attribute_names]
attrs = [product['attributes'][attr]
for attr in attribute_names if attr in product['attributes']]
Copy link
Contributor Author

@bclodius bclodius May 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I added the conditional here to work around the database_edition parameter required to avoid hash collisions; but not always existing for some database engines.

garrettheel
garrettheel approved these changes May 22, 2017
View reviewed changes
Copy link
Contributor

@garrettheel garrettheel left a comment

Choose a reason for hiding this comment

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

Awesome, thanks again for contributing! I'll get a release out with this soon

@garrettheel garrettheel merged commit 9832931 into lyft:master May 22, 2017
@bclodius
Copy link
Contributor Author

Thanks 👍 I plan to add more in the coming months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garrettheel garrettheel garrettheel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bclodius @garrettheel