-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
api: set last index and request time on alloc stop #16319
Conversation
Some of the methods in `Allocations()` incorrectly use the `putQuery` in API calls where `put` is more appropriate since they are not reading information back. These methods are also not returning request metadata such as `LastIndex` back to callers, which can be useful to have in some scenarios. They also provide poor developer experience as they take an `*api.Allocation` struct when only the allocation ID is necessary. This can lead consumers to make unnecessary API calls to fetch the full allocation. Fixing these problems require updating the methods' signatures so they take `*WriteOptions` instead of `*QueryOptions` and return `*WriteMeta`, but this is a breaking change that requires advanced notice to consumers. This commit adds a future breaking change notice and also fixes the `Stop` method so it properly returns request metadata in a backwards compatible way.
@@ -0,0 +1,3 @@ | |||
```release-note:bug | |||
api: Fix `Allocations().Stop()` method to properly set the request `LastIndex` and `RequestTime` in the response |
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.
This is the only user facing change, but should I note the future breaking changes somehow?
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.
make cl
has a deprecation
option, so I think we can add the release-note:deprecation
to this file for those.
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.
Ah nice. I added the deprecation notes and I also remembered that I added RestartAllTasks()
method to prevent a breaking change. Since we're breaking things already we may as well clean that up.
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.
I've left an item to fix but once that's accepted LGTM!
@@ -0,0 +1,3 @@ | |||
```release-note:bug | |||
api: Fix `Allocations().Stop()` method to properly set the request `LastIndex` and `RequestTime` in the response |
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.
make cl
has a deprecation
option, so I think we can add the release-note:deprecation
to this file for those.
Some of the methods in `Allocations()` incorrectly use the `putQuery` in API calls where `put` is more appropriate since they are not reading information back. These methods are also not returning request metadata such as `LastIndex` back to callers, which can be useful to have in some scenarios. They also provide poor developer experience as they take an `*api.Allocation` struct when only the allocation ID is necessary. This can lead consumers to make unnecessary API calls to fetch the full allocation. Fixing these problems require updating the methods' signatures so they take `*WriteOptions` instead of `*QueryOptions` and return `*WriteMeta`, but this is a breaking change that requires advanced notice to consumers. This commit adds a future breaking change notice and also fixes the `Stop` method so it properly returns request metadata in a backwards compatible way.
Some of the methods in
Allocations()
incorrectly use theputQuery
in API calls whereput
is more appropriate since they are not reading information back. These methods are also not returning request metadata such asLastIndex
back to callers, which can be useful to have in some scenarios.They also provide poor developer experience as they take an
*api.Allocation
struct when only the allocation ID is necessary. This can lead consumers to make unnecessary API calls to fetch the full allocation.Fixing these problems require updating the methods' signatures so they take
*WriteOptions
instead of*QueryOptions
and return*WriteMeta
, but this is a breaking change that requires advanced notice to consumers.This commit adds a future breaking change notice and also fixes the
Stop
method so it properly returns request metadata in a backwards compatible way.