Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix the # for ELB #76

Merged
merged 14 commits into from
Jul 6, 2020
Merged

Fix the # for ELB #76

merged 14 commits into from
Jul 6, 2020

Conversation

cregev
Copy link
Contributor

@cregev cregev commented Jun 30, 2020

What type of PR is this?
Bug

What this PR does / why we need it:
Fix the # of ELB
Which issue(s) this PR fixes (if exists):
Today we are calculating the price of ELB as an Application Load Balancer which is wrong.

@cregev cregev requested a review from kaplanelad June 30, 2020 09:07
@cregev cregev self-assigned this Jun 30, 2020
@cregev cregev added the kind/new-feature New feature label Jun 30, 2020
Field: awsClient.String("group"),
Value: awsClient.String("ELB:Balancer"),
Field: awsClient.String("groupDescription"),
Value: awsClient.String("LoadBalancer hourly usage by Classic Load Balancer"),
Copy link
Contributor

Choose a reason for hiding this comment

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

There is not better way to filter the price?
I not like the way that we are filter by *description field

@@ -89,10 +93,13 @@ func (el *ELBV2Manager) Detect() ([]DetectedELBV2, error) {
now := time.Now()

for _, instance := range instances {
var cloudWatchNameSpace string
if cloudWatchNS, found := loadBalancerCWNameSpace[*instance.Type]; found {
Copy link
Contributor

Choose a reason for hiding this comment

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

what are you expecting to happen when the instance type not found in loadBalancerCWNameSpace

func (el *ELBV2Manager) Get#FilterInput() *#.GetProductsInput {
func (el *ELBV2Manager) Get#FilterInput(loadbalancer *elbv2.LoadBalancer) *#.GetProductsInput {

var loadBalancerProductFamily string
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case you already know which type is it (you have a logic that take from loadBalancerCWNameSpace ).
is incase loadBalancerCWNameSpace == map[string]string
you can change it to
map[string]strcut
then you can remove this switch case

Type: awsClient.String("TERM_MATCH"),
Field: awsClient.String("group"),
Value: awsClient.String("ELB:Balancer"),
Field: awsClient.String("groupDescription"),
Copy link
Contributor

Choose a reason for hiding this comment

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

same question in elb.go

@cregev cregev requested a review from kaplanelad July 3, 2020 07:59
@kaplanelad
Copy link
Contributor

@cregev fix the conflicts

@cregev cregev requested a review from kaplanelad July 5, 2020 11:59
@cregev cregev added kind/bug Something isn't working and removed kind/new-feature New feature labels Jul 5, 2020
@codecov
Copy link

codecov bot commented Jul 5, 2020

Codecov Report

Merging #76 into master will increase coverage by 0.21%.
The diff coverage is 95.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   79.83%   80.05%   +0.21%     
==========================================
  Files          35       35              
  Lines        2624     2652      +28     
==========================================
+ Hits         2095     2123      +28     
  Misses        434      434              
  Partials       95       95              
Impacted Files Coverage Δ
collector/aws/#.go 54.54% <77.77%> (+7.57%) ⬆️
collector/aws/elb.go 91.33% <100.00%> (+0.48%) ⬆️
collector/aws/elbv2.go 91.50% <100.00%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c0e038...c20cd61. Read the comment docs.

@cregev cregev added the lgtm label Jul 6, 2020
@cregev cregev merged commit 00d09c2 into master Jul 6, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/bug Something isn't working lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants