fix(814) Add instance disks resource to linode #911
Conversation
titanventura
requested review from
mlabouardy and
ShubhamPalriwala
as code owners
| } | ||
|
|
||
| log.WithFields(log.Fields{ | ||
| "provider": "Linode", | ||
| "account": client.Name, | ||
| "service": "Linode", | ||
| "service": "Linode and Instance Disk", |
There was a problem hiding this comment.
| "service": "Linode and Instance Disk", | |
| "service": "Linode Instance and Instance Disk", |
There was a problem hiding this comment.
Usually, they call linode instances as linodes. But if linode instance and instance disk seems to sound good, we can go with that !
WDYT ?
There was a problem hiding this comment.
I'll leave it upto you if you say so I dont mind. Also did you check it in dashboard if the Linodes and Instance disks are separetly created as two different entity?
There was a problem hiding this comment.
Hey, I think it won't be displayed in the dashboard because the FetchDataFunction for Linode is actually commented out, IDK why
Reference:
komiser/providers/linode/linode.go
Line 21 in 57bbd94
There was a problem hiding this comment.
You would have to uncomment that
| resources = append(resources, models.Resource{ | ||
| Provider: "Linode", | ||
| Account: client.Name, | ||
| Service: "Instance Disk", |
There was a problem hiding this comment.
Should we rename this to Linodes Instance disk as the Service field is generally showed on Komiser dashboard, so do you think specifying is better?
Signed-off-by: titanventura <aswath7862001@gmail.com>
There was a problem hiding this comment.
Hey @titanventura, the PR looks good to me! I just think you forgot to call the service so once that is fixed, we can merge the PR!
providers/linode/linode.go
Outdated
| "github.com/tailwarden/komiser/providers/linode/storage" | ||
| "github.com/uptrace/bun" | ||
| ) | ||
|
|
||
| func listOfSupportedServices() []providers.FetchDataFunction { | ||
| return []providers.FetchDataFunction{ | ||
| // compute.Linodes, | ||
| // compute.LinodeInstancesAndInstanceDisks, |
There was a problem hiding this comment.
Did you forget to uncomment this?
There was a problem hiding this comment.
this line was commented before i started working on this PR, i changed the name of the function so made changes to this line as well, anyways the problem is that compute.Linodes or compute.LinodeInstancesAndInstanceDisks does not comply with providers.FetchDataFunction interface, because of the third parameter !
There was a problem hiding this comment.
any update on fixing this @titanventura ?
Signed-off-by: titanventura <aswath7862001@gmail.com>
|
Enabled the FetchData function for Instance and Instance disks and also implemented changes suggested in comments. Please review @AvineshTripathi @mlabouardy @ShubhamPalriwala . Also, had one more suggestion, currently, the instance disks are dependant on the instance resources in linode, as a result of this, the fetching of instance disks happens synchronously, should we have an option in komiser that enables concurrent fetching of nested reources. If we implement this, a similar pattern can be followed for other nested resources as well. Adding some concurrency would improve the fetching speed. Just a suggestion, if approved, would be happy to try this in a separate PR for the same resource. :) |
|
Hey @titanventura we are currently looking to redesign the arch of komiser where one of most important things to add is concurrent fetching of resources. If you are willing to help me with brainstorming how we do that all together would definitely like to collaborate. |
|
For sure @AvineshTripathi , I'd be more than happy to collaborate! |
|
hey @titanventura can yo dm me over discord n we connect n work on arch/ |
Problem
Support instance disks - Linode
Solution
Added instance disks to the list of resources under linodes function.
Changes Made
Checklist
Reviewers
@jakepage91 @mlabouardy
fixes #814