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

chore(gke): Remove deprecated method #350

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pokom
Copy link
Contributor

@Pokom Pokom commented Nov 4, 2024

In #344, deprecated code was discovered which led to some confusion. It wasn't clear exactly how to remove the deprecated code and ensure tests were still functioning.

  1. Rename GeneratePricingMap to ParseSkus
  2. Update ParseSkus to be a receiver method on PricingMaps
  3. Update pricingMap.Populate to return the result of ParseSkus
  4. Update pricing map tests to call ParseSkus instead

In #344, deprecated code was discovered which led to some confusion.
It wasn't clear exactly how to remove the deprecated code and ensure tests were still functioning.
1. Rename `GeneratePricingMap` to `ParseSkus`
2. Update `ParseSkus` to be a receiver method on PricingMaps
3. Update `pricingMap.Populate` to return the result of ParseSkus
4. Update pricing map tests to call ParseSkus instead
@Pokom Pokom requested a review from a team as a code owner November 4, 2024 20:59
Comment on lines +279 to +282
if strings.Contains(data.Description, "Confidential") {
log.Printf("Storage class contains Confidential: %s\n%s\n", storageClass, data.Description)
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm moving this after the first storageClass check since that one should take priority.

Comment on lines +643 to +646
pricingMap := &PricingMap{
Compute: make(map[string]*FamilyPricing),
Storage: make(map[string]*StoragePricing),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically not calling NewPricingMap(...) because that method will also call populate. I'm specifically trying to avoid having to mock out the billing client.

Copy link

@cindy cindy left a comment

Choose a reason for hiding this comment

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

This change looks good to me, but I would be open to having someone else review it as well.

Copy link
Member

@the-it the-it left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants