-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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 Rancher API pagination limits #1453
Fix Rancher API pagination limits #1453
Conversation
provider/rancher.go
Outdated
@@ -290,7 +290,10 @@ func listRancherEnvironments(client *rancher.RancherClient) []*rancher.Environme | |||
|
|||
var environmentList = []*rancher.Environment{} | |||
|
|||
environments, err := client.Environment.List(nil) | |||
withoutPagination := &rancher.ListOpts{ |
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.
How about setting up a "const" ListOpts
and reuse it?
(where "const" could possibly mean: declare a var and initialize it in an init()
function.)
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.
Sure @timoreimann. Like this?
Could we cover the disabled pagination in one of our integration tests somehow? Not a must if it turns out to be tricky, but would be nice. |
provider/rancher.go
Outdated
@@ -44,6 +48,10 @@ type rancherData struct { | |||
Health string | |||
} | |||
|
|||
func init() { | |||
withoutPagination.Filters["limit"] = 0 |
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.
You're accessing the fields of a pointer to a struct. Wouldn't that cause a nil dereference error unless you initialized the variable?
It may be tricky as it's a struct passed straight into the Rancher client but happy to take a look @timoreimann. Could you point me towards the integration tests for Rancher though? I couldn't find them 😕 |
@martinbaillie: apologies -- I thought we had integration tests for Ranger but apparently we don't. IIRC, it's on the todo list. Let's leave that for another PR then. |
This fix allows the Traefik Rancher provider to obtain a complete view of the environments, services and containers being managed by the Rancher deployment.
bf331f7
to
73f09f3
Compare
@timoreimann no problem re. integration tests. I've tidied up the branch commits and fixed that init mistake 🤒 |
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.
LGTM, thanks.
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.
LGTM 🐯
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.
LGTM 👼 Thanks! Could you fix the conflict so we can merge? @martinbaillie
@SantoDE merged in master again. cheers 👍 |
This fix allows the Traefik Rancher provider to obtain a complete view
of the environments, services and containers being managed by the
Rancher deployment.
Resolves #1452