Fix issue when gitlab return only first 20 migrations #379
Conversation
Pull Request Test Coverage Report for Build 812
💛 - Coveralls |
| return err | ||
| } | ||
|
|
||
| if response.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
Note, this will fail the whole readDirectory() call if a single request in the request chain fails.
To reduce the chances of that happening, it may be worth defaulting the per_page option to 100 (the max) and making that configurable.
There was a problem hiding this comment.
How would you recommend to configure that parameter?
There was a problem hiding this comment.
Oh, I mean how to pass that parameter from migrate.go to a particular gitlab driver. Do you think extending Gitlab.url string is a good idea? For instance, gitlab://username:token@gitlab.com/11197284/migrations?per_page=50
There was a problem hiding this comment.
If you're using migrate via the CLI, the only way to configure the source driver is via URL query params. We try our best to avoid conflicts with the underlying driver, but for gitlab, that doesn't seem like it'll be an issue.
If you're using migrate as a Go library, the driver can be configured via WithInstance(). It looks like gitlab isn't properly using Config
| @@ -1,12 +1,13 @@ | |||
| # gitlab | |||
|
|
|||
| `gitlab://user:personal-access-token@gitlab_url/project_id/path#ref` | |||
| `gitlab://user:personal-access-token@gitlab_url/project_id/path#ref?per_page=20` | |||
There was a problem hiding this comment.
This is inaccurate. The query precedes the fragment: https://golang.org/pkg/net/url/#URL
| listOptions *gitlab.ListTreeOptions | ||
| getOptions *gitlab.GetFileOptions | ||
| migrations *source.Migrations | ||
| } | ||
|
|
||
| type Config struct { | ||
| Scheme string |
There was a problem hiding this comment.
Does the scheme need to be specified? When would it not be https?
There was a problem hiding this comment.
For local (in-house) Gitlab instances it's up to you to choose a scheme. Our internal network doesn't provide https access to Gitlab, only http.
There was a problem hiding this comment.
When you set the Scheme via URL e.g. in Open() you can't use u.Scheme since that'll be gitlab, not http or https. The scheme will need to be provided via a x-scheme URL query parameter.
| @@ -120,8 +132,16 @@ func WithInstance(client *gitlab.Client, config *Config) (source.Driver, error) | |||
| func (g *Gitlab) readDirectory() error { | |||
| var nodes []*gitlab.TreeNode | |||
|
|
|||
| g.listOptions = &gitlab.ListTreeOptions{ | |||
There was a problem hiding this comment.
readDirectory() shouldn't modify the receiver e.g. g *Gitlab
Instead, the Gitlab instance should be configured upfront when it's created
| @@ -153,6 +173,13 @@ func (g *Gitlab) readDirectory() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (g *Gitlab) ensureFields() { | |||
There was a problem hiding this comment.
Likewise, this should be called in Open() or WithInstance() instead of Gitlab methods that act on a Gitlab instance
| if m, ok := g.migrations.Up(version); ok { | ||
| f, response, err := g.client.RepositoryFiles.GetFile(g.projectID, m.Raw, g.getOptions) | ||
| g.getOptions = &gitlab.GetFileOptions{ |
There was a problem hiding this comment.
Again, don't set instance params in methods that shouldn't be modifying the instance
| if m, ok := g.migrations.Down(version); ok { | ||
| f, response, err := g.client.RepositoryFiles.GetFile(g.projectID, m.Raw, g.getOptions) | ||
| g.getOptions = &gitlab.GetFileOptions{ |
| } else { | ||
| return v, nil | ||
| } | ||
| } | ||
|
|
||
| func (g *Gitlab) ReadUp(version uint) (r io.ReadCloser, identifier string, err error) { | ||
| g.ensureFields() |
| } | ||
|
|
||
| func (g *Gitlab) ReadDown(version uint) (r io.ReadCloser, identifier string, err error) { | ||
| g.ensureFields() |
|
Any news? |
|
Superseded by #497 |
When you are trying to fetch more than 20 migrations response headers look like on this picture:
That means that there are more than one result page.
https://gitlab.com/help/api/README.md#pagination