feat: adds support for migrations #14
Conversation
gene-redpanda
force-pushed
the
add-migrations
branch
from
09818d4 to
2a90304
Compare
| // ExecuteMigration executes a specific action on a migration identified by its ID. The action must be one of: | ||
| // prepare, execute, finish, cancel. | ||
| func (a *AdminAPI) ExecuteMigration(ctx context.Context, id int, action string) error { | ||
| validActions := map[string]bool{ |
There was a problem hiding this comment.
should this be just a single var declared outside of the method?
Also minor nit / optimization might be to use the empty struct for the values as it caries 0 size.
var validActions = map[string]struct{}{
"prepare": struct{}{},
"execute": struct{}{},
"finish": struct{}{},
"cancel": struct{}{},
}and then the check can be something like:
if _, valid := validActions[action]; !valid {
// ...
}There was a problem hiding this comment.
It's only used in one place so I felt like that was the right place to declare it. I can change it if needed
There was a problem hiding this comment.
Yup tradeoffs. I don't feel strongly about it either way.
|
The golangci-lint timeout is due to 1.23 Go usage and golangci-lint being an older version. We need to update the golangci-lint action and go version used for linting. |
|
@bojand I cleaned up the linter CI setup some |
There was a problem hiding this comment.
A couple of comments
btw, how did GH let us merge this one with a linter failure? https://github.com/redpanda-data/common-go/actions/runs/10511516964/job/29122660139 🤔
| // AddMigration adds a migration to the cluster. It accepts one of InboundMigration or OutboundMigration. | ||
| func (a *AdminAPI) AddMigration(ctx context.Context, migration any) (int, error) { | ||
| migrationType := reflect.TypeOf(migration) |
There was a problem hiding this comment.
To avoid having any in the input, what do you think of splitting the AddMigration to AddInboudMigration and AddOutbountMigration. It's easier for the caller to figure the usage.
| with: | ||
| version: v1.56 | ||
| version: v1.60.2 |
There was a problem hiding this comment.
Can you confirm that https://github.com/redpanda-data/common-go/actions/runs/10511516964/job/29122660139 is not related to this change?
There was a problem hiding this comment.
Issue started prior to this PR:
https://github.com/redpanda-data/common-go/actions/runs/10394862238/job/28835193918
| baseMigrationEndpoint = "/v1/migrations/" | ||
| ) | ||
|
|
||
| // AddMigration adds a migration to the cluster. It accepts one of InboundMigration or OutboundMigration. |
There was a problem hiding this comment.
Can you document what is the integer in the response means to the end user?
| var response AddMigrationResponse | ||
| if err := a.sendOne(ctx, http.MethodPut, baseMigrationEndpoint, migration, &response, false); err != nil { | ||
| return -1, err | ||
| } | ||
| return response.ID, nil |
There was a problem hiding this comment.
Why return the response.ID and not the AddMigrationResponse? it will be better if we return the struct if more information gets added to the response in the future.
Adds support for migrations to the admin api