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

Fix Rancher API pagination limits #1453

Merged

Conversation

martinbaillie
Copy link
Contributor

@martinbaillie martinbaillie commented Apr 18, 2017

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

@martinbaillie martinbaillie changed the title Fix Rancher API pagination limits (#1452) Fix Rancher API pagination limits Apr 18, 2017
@@ -290,7 +290,10 @@ func listRancherEnvironments(client *rancher.RancherClient) []*rancher.Environme

var environmentList = []*rancher.Environment{}

environments, err := client.Environment.List(nil)
withoutPagination := &rancher.ListOpts{
Copy link
Contributor

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @timoreimann. Like this?

@timoreimann
Copy link
Contributor

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.

@@ -44,6 +48,10 @@ type rancherData struct {
Health string
}

func init() {
withoutPagination.Filters["limit"] = 0
Copy link
Contributor

@timoreimann timoreimann Apr 18, 2017

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?

@martinbaillie
Copy link
Contributor Author

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 😕

@timoreimann
Copy link
Contributor

@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.
@martinbaillie martinbaillie force-pushed the rancher-provider-pagination-fixes branch from bf331f7 to 73f09f3 Compare April 18, 2017 09:50
@martinbaillie
Copy link
Contributor Author

@timoreimann no problem re. integration tests.

I've tidied up the branch commits and fixed that init mistake 🤒

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

Copy link
Collaborator

@SantoDE SantoDE left a 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

@martinbaillie
Copy link
Contributor Author

@SantoDE merged in master again. cheers 👍

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

Successfully merging this pull request may close these issues.

Traefik does not pull all environments/services/containers from the Rancher API
5 participants