-
Notifications
You must be signed in to change notification settings - Fork 431
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
Fix issue(#823) mysql-instance #860
Fix issue(#823) mysql-instance #860
Conversation
fil-issue(tailwarden#823)
To display cost as per usage.
Hey @ShubhamPalriwala, I tried to add and determine cost of client of SQL as per the metadata from the link you provided. |
Hey @professorabhay, you missed calling the resource, please take a look at this amazing video by @jakepage91: https://www.youtube.com/watch?v=Vn5uc2elcVg This adds a similar feature to yours and you will know how to call it and possibly add tests if possible! |
imported sql.go
//make call to the `GetSQLInstances` function of /sql/sql.go
Hey @ShubhamPalriwala, I checked that video and its really helpful. And did the needed changes. |
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 PR looks amazing now! 🚀 Thanks a ton!
One small nit from my end, if you wanna make the code even cleaner:
I took a deeper look, and the the pricing for:
- Dedicated x GB is of the form:
- 1 Node: y
- 3 Node: 3*Y
- Shared CPU x GB is of the form:
- 1 Node: z
- 3 Node: z*(2.333)
So you can prolly just store the single node price and then see if the API returns 1 node or 3 node and then do cost calculation accordingly in a function! This reduces the hardcoding of values and helps in easier updates to the cost if needed.
Let me know if you want to take this up else I can help you out with this! 🥳
Hai @ShubhamPalriwala, I can modify the code accordingly. |
Hey @ShubhamPalriwala, I tried to modify the file. |
Changes Function name
Hey @mlabouardy, I make the changes. |
Hey I just took a look and this approach: if strings.HasPrefix(instanceType, "Dedicated") {
cost, ok := dedicatedCPUCosts[instanceType]
if !ok {
return 0, false
}
// Adjust cost for 3 Node instances
if nodeCount == 3 {
cost *= 3
}
} else if strings.HasPrefix(instanceType, "Shared") {
cost, ok := sharedCPUCosts[instanceType]
if !ok {
return 0, false
}
// Adjust cost for 3 Node instances
if nodeCount == 3 {
cost *= 2.333
}
} else {
return 0, false
} lgtm! So you can push the changes once tested! |
Sure @ShubhamPalriwala, I will do it in some time. |
Hey @ShubhamPalriwala and @mlabouardy, Kindly take a look to the new commit !! 🙂 |
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.
Thanks a lot @professorabhay, the PR looks amazing to me now! Just ran the CI, if that passes then over to you @mlabouardy 🚀
Hey @ShubhamPalriwala, Is all the test pass or not?? |
Hey @professorabhay, I see the build failing in the CI! I'm taking a look rn |
Hey, did you test this locally? I can also reproduce the build error locally from below: // Instances fetches SQL instances from the provider and returns them as resources.
func Instances(ctx context.Context, client providers.ProviderClient) ([]models.Resource, error) {
resources := make([]models.Resource, 0)
instances, err := client.SQLClient.GetInstances(ctx)
if err != nil {
return resources, err
}
...
} A good start to debugging this is to use the correct client ie |
Sure, I am looking into it. |
Hey @professorabhay, let me know if you need a hand with this :) so we can merge it this week! |
Hello @ShubhamPalriwala, It seems that the NodeCount value is not available in the Linode cloud's API or in the linodego.Instance struct. How, Can I resolve it ? |
Hey @professorabhay, try exploring the metadata you receive in the instance struct and see if the API provides the number of nodes anywhere? If then we can write a method to parse it accordingly, else we would then need to find a different identifier for the number of nodes for cost calculation! Let me know if you tried and aren't able to at all, then I'll try to go hands-on with it |
Yes @ShubhamPalriwala , I took a look into the Linode API and there is nothing like node count and similar like this. Instead of count there is some sort of ID's. |
Hey @professorabhay, I just went through your PR locally. A couple of pointers:
You have already did most of the hardwork so the above pointers should help you, if not, lets hop on Discord sometime and get things going 🏃🏼 |
Hey @ShubhamPalriwala, I tried it out as you said. |
Hey @professorabhay, I took a quick glance and here are my suggestions:
|
Hey @ShubhamPalriwala, I'll keep all the point in mind and come with an update soon. |
Hey @ShubhamPalriwala, Please take a look to latest commit here - professorabhay#5 |
Hey @professorabhay, I'm unable to see the relevant commit there, I would recommend once you test it locally, please push it here on this PR branch and I'd be happy to review it |
Hey @ShubhamPalriwala, Take a look |
Problem
Need support for Linode MySQL instance
Solution
Create a sql.go file to collect MySQL Instances.
Changes Made
1- created a dir and file -> /sql/sql.go
2- modify instance.go to determine cost
Reviewers
@ShubhamPalriwala @jakepage91